From 1eff1b9f9c3a432b4fd43093952e1100695ad49b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 06:51:57 +0000 Subject: [PATCH 1/8] Add a post-migration review of the coverage and suggestion path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audits the shipped native app against the phase plans, diffing every Kotlin port against the TypeScript original recovered from git history (legacy-web/ was deleted in 3e11ea8 but survives in the log). The ported maths is faithful — the engines, the tie-break order, the 0.5/1.0 weights and the deliberate generationIntroduced deviation all match. The defects are at the seams: - regenerateSlot() sorts with a selector that re-rolls its random noise on every comparison, so the comparator breaks Comparator's contract: TimSort throws on pools above MIN_MERGE, and the "top 5" is arbitrary even when it doesn't. A deviation from teamGenerator.ts, which scored each candidate once. Includes measured throw rates from a standalone JVM reproduction. - Both engines run on Dispatchers.Main.immediate — the Analysis pipeline through stateIn(viewModelScope), the generator through plain non-suspend click handlers with no progress indicator. - computeCompositeScore rebuilds the per-team half of its work once per candidate instead of once per team. - The Suggestions panel dropped the PWA's type filter, random mode and two contextual messages, and shows 5 cards where the original showed 10. - Surprise Me's "Custom slots" stepper consumes the six-slot budget and places nothing: the generator never reads customSlots. - Abilities are honoured by the coverage grid but ignored by the suggestion scoring, so one screen contradicts itself. Also notes that 0 of 59 items in docs/test-plan.md are ticked; three of the six findings are ones a single on-device pass would have surfaced. Documentation only, no behaviour changes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018zXVpxEbSWadqKyntXTXD6 --- CHANGELOG.md | 7 + docs/post-migration-review.md | 282 ++++++++++++++++++++++++++++++++++ 2 files changed, 289 insertions(+) create mode 100644 docs/post-migration-review.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 7da4379..74c884b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,13 @@ versions follow [Semantic Versioning](https://semver.org/). ## [Unreleased] +- **Post-migration review of the coverage and suggestion engines.** A + code-level audit of the shipped app against the phase plans, diffing every + Kotlin port against the TypeScript original recovered from git history. + Records six findings and an ordered remediation plan in + [`docs/post-migration-review.md`](docs/post-migration-review.md); + no behaviour changes in this entry. + ## [2.0.0] - 2026-09-04 CoverDex is now a native Android app — the full six-phase rewrite diff --git a/docs/post-migration-review.md b/docs/post-migration-review.md new file mode 100644 index 0000000..9da8967 --- /dev/null +++ b/docs/post-migration-review.md @@ -0,0 +1,282 @@ +# Post-migration review — coverage and suggestions + +A code-level audit of the shipped native app against +[`docs/plan/native-spec.md`](plan/native-spec.md) and the phase plans, run +after Phase 6 closed. The focus, as requested, is the coverage engine and +the suggestion/generator path; the other subsystems were swept for +concrete defects rather than re-reviewed line by line. + +**Method.** `legacy-web/` was deleted in `3e11ea8`, but it survives in git +history, so every Kotlin port was diffed against its TypeScript original +recovered with `git show 3e11ea8^:legacy-web/...`. That is the same oracle +Phase 6 used, applied a second time and independently. + +**What was not done.** This sandbox has no Android SDK (`ANDROID_HOME` is +empty), so nothing here was compiled or run. Every claim below is either a +source-level reading, a diff against the recovered TypeScript, or a +standalone JVM reproduction that is named as such. No wall-clock figure was +measured on a device. + +## Verdict + +The engines are ported **faithfully**. `CoverageEngine.kt`, +`AbilityEffects.kt`, `Scoring.kt` and `SuggestionEngine.kt` match their +TypeScript originals function for function, including the tie-break order, +the `0.5`/`1.0` weights, and the deliberate `generationIntroduced` +deviation. The 78 domain unit tests are genuinely thorough. Nothing in the +maths is wrong. + +The defects are all **at the seams**: one port that silently changed a +sort into a non-deterministic one, two engines invoked on the main thread, +and a slice of the suggestion UI that was never carried over. None of these +is reachable by the existing unit tests, for a structural reason worth +stating plainly: the test pools are 5–20 entries, the production pool is +several hundred, and no test asserts anything about threading. + +Separately: **0 of 59 items in [`docs/test-plan.md`](test-plan.md) are +ticked.** Every phase is marked ✅ in `CLAUDE.md` on the strength of CI and +unit tests alone. Three of the five findings below are exactly the kind a +single pass on a real device would have caught immediately. + +## Findings + +### 1. `regenerateSlot` can crash, and its "top 5" is not the top 5 + +`domain/generator/TeamGenerator.kt:228` + +```kotlin +val scored = candidatePool + .map { entry -> entry to memberFromEntry(entry) } + .sortedByDescending { (_, member) -> computeScore(chart, member, otherMembers, random) } +``` + +`computeScore` adds fresh random noise on every call +(`TeamGenerator.kt:88`). Kotlin's `sortedByDescending` expands to +`sortedWith(compareByDescending(selector))`, and `compareByDescending` +invokes the selector **once per comparison per operand** — it does not +memoize. Two consequences: + +- **The comparator is non-deterministic**, so it violates + `Comparator`'s transitivity contract. `sortedWith` routes to + `java.util.Collections.sort` → `Arrays.sort` → TimSort, which detects + broken merge invariants and throws + `IllegalArgumentException: Comparison method violates its general + contract!`. `SurpriseMeViewModel.regenerateSlot` is called straight from + a Compose `onClick` with nothing catching it, so this is an app crash on + the per-slot reroll button. +- **The ordering is meaningless even when it doesn't throw**, so + `scored[random.nextInt(topN)]` picks from an arbitrary five, not the five + best. + +Composite scores land on a 0.5 lattice while the noise is ±0.01, so the +noise *only* ever reorders exact ties — within a tie group the comparator +is a coin flip, which is the worst case for TimSort. A standalone JVM +reproduction of that distribution (JDK 21; Android's libcore uses the same +TimSort) throws at these rates: + +| pool size | distinct scores = 1 | = 4 | = 20 | +|---|---|---|---| +| 32 | 0.6% | 0.6% | 0% | +| 128 | 5.4% | 4.4% | 0.4% | +| 800 | 10.6% | 6.0% | 2.2% | + +The production pool after `buildEligiblePool` is in the high hundreds +(568 final-evolution *species*, plus the alternate forms of those species — +see [`reference-pokedata.md`](plan/reference-pokedata.md) §"final-evolution +derivation"). The existing tests pass because their pools are under +TimSort's `MIN_MERGE = 32` threshold, below which it uses binary insertion +sort and never checks the contract. + +This is also a **deviation from the original**, not an inherited bug: +`teamGenerator.ts` scored each candidate exactly once into a `{entry, +member, score}` triple and sorted on the stored `score`. Scoring on the +fly was introduced by the port. + +`generateTeam:172` uses `maxByOrNull`, which calls the selector once per +element, and is therefore correct. + +**Fix.** Materialize the score, then sort: + +```kotlin +val scored = candidatePool + .map { entry -> Triple(entry, memberFromEntry(entry), computeScore(chart, memberFromEntry(entry), otherMembers, random)) } + .sortedByDescending { it.third } +``` + +Also drops the cost from O(n log n) scorings to O(n). + +**Test.** A `regenerateSlot` case with a pool of ≥100 entries whose +composite scores mostly tie, run in a loop; it fails today and passes after. + +### 2. Both engines run on the main thread + +- `ui/team/analysis/AnalysisViewModel.kt:84-130` — `analyseTeam`, + `sharedWeaknessCounts` and `computeSuggestions` all run inside the + `combine` transform. `stateIn(scope = viewModelScope, …)` collects on + that scope's context, which is `SupervisorJob() + Dispatchers.Main.immediate`. +- `ui/surprise/SurpriseMeViewModel.kt:104,113` — `generate()` and + `regenerateSlot()` are plain non-suspend functions invoked directly from + Compose click handlers. + +There is no `withContext`, `flowOn` or `Dispatchers.Default` anywhere under +`ui/`. + +Order of magnitude, per invocation, for a full team of six and a pool of +N ≈ 800: + +- **Suggestions, replacement mode** — per candidate, 6 leave-one-out + passes, each ≈ 432 type-chart lookups and ~12 `Set` allocations → + **≈ 2.3M lookups and ~65k allocations per emission**, re-run on every + team edit, toggle flip and generation-filter change. +- **`generateTeam`** — 6 slots × N candidates × a full team-coverage + recomputation each, in one synchronous click handler with no progress + indicator. + +Wall-clock on a device is unmeasured, but this is well past the 16 ms frame +budget and the generator is a plausible ANR on a low-end phone. + +**Fix.** Make the two `SurpriseMeViewModel` entry points `suspend` bodies +launched on `Dispatchers.Default` with an `isGenerating` flag driving a +progress indicator, and add `.flowOn(Dispatchers.Default)` to the +`AnalysisViewModel` pipeline (plus a debounce on the filter flows). +`CoroutinesModule` already exists as the place to inject the dispatcher, +which also makes it swappable in tests. + +### 3. 83% of the suggestion work is recomputed per candidate + +`domain/suggestion/Scoring.kt:52-70`. `computeCompositeScore` recomputes +`baseCov` and `otherWeaknessMap` from `otherMembers` on every call — but +those depend only on the team, not on the candidate, so they are identical +across all N candidates. In replacement mode there are just six distinct +`otherMembers` sets; today they are rebuilt N × 6 times instead of 6. + +That is ~360 of the ~432 lookups per candidate-pass. + +**Fix.** Hoist the per-team part into a precomputed context passed into the +scorer, computed once (addition mode) or six times (replacement mode). +Roughly a 5× reduction, and it composes with finding 2 rather than +replacing it. Behaviour-preserving: the existing +`SuggestionEngineTest`/`TeamGeneratorTest` suites are the regression net. + +### 4. The Suggestions panel is missing features the PWA shipped + +Diffing `SuggestionPanel.android.tsx` / `SuggestionFilters.android.tsx` +against `ui/team/analysis/SuggestionFilters.kt` and `AnalysisScreen.kt:155`: + +| PWA behaviour | Native | +|---|---| +| Type filter chips (`filterByType`) — narrow suggestions to chosen types | **missing** | +| "Best coverage" / "Random" mode toggle + "Randomize again" (`pickRandom(filtered, 10)`) | **missing** | +| `suggestions.noMatch` — "No suggestions match current filters." | **missing** | +| `suggestions.solidCoverage` — shown when every displayed card has `gain == 0` | **missing** | +| Shows `filtered.slice(0, 10)` | shows `take(5)` | + +The corresponding keys are absent from both `res/values/strings.xml` and +`res/values-en/strings.xml` (`bestCoverage`, `random`, `randomizeAgain`, +`noMatch`, `solidCoverage`, `newTypesCovered`). The mixed-moves note *was* +ported (`analysis_basis_mixed`). + +`computeSuggestions` already returns every ranked candidate uncapped, so +the type filter and the random mode are pure UI work over data that is +already there. + +Also orphaned: `suggestions_exclude_legendaries` is defined in both locale +files and referenced by no composable — the toggle actually lives in +Settings under the inverted `settings_include_legendaries` framing. + +**Fix.** Restore the two filters and the two contextual messages, raise the +cut to 10, and either wire up or delete the orphan string. Whether the +"random" mode is still wanted is a product call; if not, say so in +`implementation-decisions.md` rather than leaving it undocumented. + +### 5. The "Custom slots" stepper does nothing + +`ui/surprise/SurpriseMeScreen.kt:156-159` renders a stepper bound to +`GeneratorConstraints.customSlots`. `TeamGenerator.kt` never reads that +field — the port deliberately dropped the `customs` parameter because it +was dead in `teamGenerator.ts` too (documented in +`implementation-decisions.md`, "Phase 4"), and `customSlots` was kept only +"as a ported struct field". + +But `SurpriseMeUiState.constraintTotal` counts it, so the stepper actively +consumes the six-slot budget (`remainingSlots`, `budgetFull`) and can block +the user from allocating starter/legendary slots — in exchange for zero +custom Pokémon ever being placed. `SurpriseMeViewModel` does load +`customs` into its UI state and then never uses it. + +This is shipped UI that lies to the user, which is worse than the dead +parameter it came from. + +**Fix.** Either implement it — reserve N slots filled from +`CustomPokemonRepository.roster`, scored the same way — or remove the +stepper and the `customSlots` field. Implementing is the better call: it +is the one generator feature that serves this app's stated ROM-hack/draft +audience, and the roster is already in the ViewModel. + +### 6. Abilities are honoured in the grid but ignored when scoring + +`Scoring.kt:24` computes `weaknesses` with no ability argument, faithfully +porting `getWeaknesses`. `sharedWeaknessCounts` +(`CoverageEngine.kt:161`) does pass `m.ability`. Both feed the same +Analysis screen, so it can simultaneously report that the team is *not* +weak to Ground (Levitate) and penalise a Ground-weak candidate for +"aggravating" that weakness. + +The candidate side compounds it: `memberFromEntry` sets `ability = null`, +but `applySuggestion` writes the species' `defaultAbility` — so a suggested +Pokémon is scored without the ability it is about to be given. + +Inherited from the PWA, so not a migration regression, but it is a genuine +inaccuracy and the two halves of one screen disagreeing is the visible +symptom. + +**Fix.** Pass the ability through `weaknesses()`, and give `memberFromEntry` +the entry's `defaultAbility`. This *changes scores* — it is a spec change, +not a refactor, so it needs an `implementation-decisions.md` entry and +updated test expectations in the same commit. Worth doing; do it on its own. + +## Plan + +Ordered by risk removed per unit of work. + +**Now — correctness** + +1. Finding 1: memoize the score in `regenerateSlot`, plus the large-pool + regression test. One-line fix, removes a crash. +2. Finding 5: decide and act on `customSlots` — implement, or remove the + stepper. Currently misleading either way. + +**Next — responsiveness** + +3. Finding 2: move both engines off the main thread; add the generator's + progress indicator. +4. Finding 3: hoist the per-team precomputation out of the candidate loop. + +Doing 4 before 3 is tempting and wrong: getting the work off the main +thread is what fixes the jank, and the optimisation is then a bonus rather +than load-bearing. + +**Then — parity and accuracy** + +5. Finding 4: restore the type filter, the two contextual messages and the + 10-card cut; decide on the random mode; clear the orphan string. +6. Finding 6: thread abilities through the scoring path, as a documented + spec change. + +**Underneath all of it** + +7. Work `docs/test-plan.md` on a real device. 59 unchecked items is the + single largest gap in confidence in this repository, and findings 1, 2 + and 5 are all things a first run-through would have surfaced. +8. Add the two test shapes that would have caught these: an engine test at + production pool scale (several hundred entries), and a ViewModel test + asserting the engines are not invoked on the collecting dispatcher. + +## Areas swept, no defects found + +Ported maths (`CoverageEngine.kt`, `AbilityEffects.kt`, `Scoring.kt`, +`SuggestionEngine.kt`) against the recovered TypeScript; the suggestion +ranking comparator; `analyseTeam`'s mixed-mode handling; `generateTeam`'s +quota logic; `applySuggestion`'s slot resolution (custom roster entries +always carry `pokedexId = null`, so a custom's ability is not overwritten); +the type chart's 18×18 completeness. From bcd37f5d399875bae5e073194ba3dd2153844f7b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 07:01:32 +0000 Subject: [PATCH 2/8] Fix regenerateSlot's non-deterministic comparator (finding 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sortedByDescending { computeScore(...) } re-invoked computeScore once per comparison, and computeScore adds fresh random noise on every call (TeamGenerator.kt's own tie-breaking factor). That makes the comparator non-transitive, which breaks Comparator's contract: TimSort detects the inconsistency and throws IllegalArgumentException once the candidate pool is large enough to leave binary-insertion-sort territory (pools under Collections.sort's MIN_MERGE = 32 never hit the check, which is why every existing test pool, all under 20 entries, passed). The production pool after buildEligiblePool is in the high hundreds. Even when it doesn't throw, the resulting order is arbitrary, so the "top 5" regenerateSlot samples from is not actually the top 5. This was introduced by the port: teamGenerator.ts scored each candidate once into a stored {entry, member, score} triple and sorted on that. generateTeam's maxByOrNull already evaluates its selector once per element and was unaffected. Fix: compute the score once per candidate into a Triple, sort on the stored value. Adds a regression test with a 300-entry same-typed pool (so composite scores tie and only the random noise breaks ties — the worst case for the old code) across 50 seeds; asserts no exception. Full analysis: docs/post-migration-review.md, finding 1. No Android SDK in this sandbox (confirmed: ANDROID_HOME empty, no sdkmanager, ./gradlew testDebugUnitTest fails at SDK resolution before compiling) — this fix and test are unverified by a local run. The approach mirrors a standalone JVM reproduction of the same TimSort/ Comparator failure mode (not committed, ad hoc), and CI is watched. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_018zXVpxEbSWadqKyntXTXD6 --- CHANGELOG.md | 11 ++++++++-- .../domain/generator/TeamGenerator.kt | 12 ++++++++-- .../domain/generator/TeamGeneratorTest.kt | 22 +++++++++++++++++++ docs/test-plan.md | 8 ++++++- 4 files changed, 48 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74c884b..5dcdda8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,12 +6,19 @@ versions follow [Semantic Versioning](https://semver.org/). ## [Unreleased] +- **Fixed a crash in "Regenerate" (Surprise Me).** `regenerateSlot`'s + candidate ranking re-evaluated its composite score (including the random + tie-breaking noise) on every comparator invocation instead of once per + candidate, which could throw `IllegalArgumentException: Comparison + method violates its general contract!` once the eligible pool was large + enough — see finding 1 in + [`docs/post-migration-review.md`](docs/post-migration-review.md). The + score is now computed once per candidate and sorted on the stored value. - **Post-migration review of the coverage and suggestion engines.** A code-level audit of the shipped app against the phase plans, diffing every Kotlin port against the TypeScript original recovered from git history. Records six findings and an ordered remediation plan in - [`docs/post-migration-review.md`](docs/post-migration-review.md); - no behaviour changes in this entry. + [`docs/post-migration-review.md`](docs/post-migration-review.md). ## [2.0.0] - 2026-09-04 diff --git a/app/src/main/java/com/marcogn/coverdex/domain/generator/TeamGenerator.kt b/app/src/main/java/com/marcogn/coverdex/domain/generator/TeamGenerator.kt index ff943cb..977b4ba 100644 --- a/app/src/main/java/com/marcogn/coverdex/domain/generator/TeamGenerator.kt +++ b/app/src/main/java/com/marcogn/coverdex/domain/generator/TeamGenerator.kt @@ -223,9 +223,17 @@ fun regenerateSlot( return currentTeam[slotIndex] } + // computeScore adds fresh random noise per call, so it must be evaluated exactly once per + // candidate here and sorted on the stored value — sortedByDescending { computeScore(...) } + // re-invokes the selector on every comparison, which breaks Comparator's contract and makes + // Collections.sort's TimSort throw once the pool is large enough to leave insertion-sort + // territory (candidatePool is the full eligible catalogue here, not a test-sized fixture). val scored = candidatePool - .map { entry -> entry to memberFromEntry(entry) } - .sortedByDescending { (_, member) -> computeScore(chart, member, otherMembers, random) } + .map { entry -> + val member = memberFromEntry(entry) + Triple(entry, member, computeScore(chart, member, otherMembers, random)) + } + .sortedByDescending { (_, _, score) -> score } val topN = minOf(5, scored.size) val picked = scored[random.nextInt(topN)] diff --git a/app/src/test/java/com/marcogn/coverdex/domain/generator/TeamGeneratorTest.kt b/app/src/test/java/com/marcogn/coverdex/domain/generator/TeamGeneratorTest.kt index 2f84070..81ea2a6 100644 --- a/app/src/test/java/com/marcogn/coverdex/domain/generator/TeamGeneratorTest.kt +++ b/app/src/test/java/com/marcogn/coverdex/domain/generator/TeamGeneratorTest.kt @@ -130,6 +130,28 @@ class TeamGeneratorTest { } } + // A same-typed synthetic pool gives every candidate an identical base composite score, so + // only computeScore's random noise breaks ties — the worst case for a comparator that must + // stay consistent across repeated evaluations. Before the fix, sortedByDescending re-rolled + // that noise on every comparison and threw "Comparison method violates its general contract!" + // for a real fraction of seeds once the pool passed TimSort's insertion-sort threshold; this + // pool (300 entries) reliably exceeds it. The assertion is that nothing above throws. + private val largeTiedScorePool: List = (1..300).map { i -> + PokemonEntry( + id = 20_000 + i, name = "mon$i", displayName = "Mon$i", speciesId = 20_000 + i, speciesName = "mon$i", + types = PokemonType.NORMAL to null, isLegendary = false, isMythical = false, isFinalEvolution = true, + generationIntroduced = 1, defaultAbility = null, isDefaultForm = true, + ) + } + + @Test + fun `regenerateSlot does not crash on a large pool of tied composite scores`() { + val team = sixMemberTeam() + for (seed in 0..49) { + regenerateSlot(chart, largeTiedScorePool, team, 5, DEFAULT_CONSTRAINTS, Random(seed)) + } + } + // ---- anchor composite score validation ---- // // teamGenerator.test.ts runs this 5 times with real Math.random() and accepts 4/5 passes. diff --git a/docs/test-plan.md b/docs/test-plan.md index 36bc53d..823e4d4 100644 --- a/docs/test-plan.md +++ b/docs/test-plan.md @@ -219,7 +219,13 @@ None yet. ### Known regressions -None yet. +- **Fixed 2026-09-05.** "Regenerate" on a single Surprise Me slot could + crash with `IllegalArgumentException: Comparison method violates its + general contract!` once the eligible candidate pool was large — found by + code review (`docs/post-migration-review.md`, finding 1), not by manual + testing; reproduced with a standalone JVM harness before the fix, not on + a device. `regenerateSlot`'s candidate ranking now scores each candidate + once instead of re-rolling the random tie-breaker on every comparison. ## Phase 5 — Showdown import/export, settings and local backup From 67a16ee92e656c74988701d0f59ebe37a866bbf9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 07:12:32 +0000 Subject: [PATCH 3/8] Implement Surprise Me's "Custom slots" (finding 5) GeneratorConstraints.customSlots has existed since Phase 4 as a ported struct field, and SurpriseMeScreen has always rendered a stepper bound to it, but TeamGenerator.kt never read the field: teamGenerator.ts accepted a customs: TeamMember[] parameter it never referenced, and the port dropped it as genuinely dead. The stepper still counted toward SurpriseMeUiState.constraintTotal/remainingSlots/budgetFull, so it could block a user from allocating starter/legendary/Mega/Dynamax slots in exchange for placing zero custom Pokemon, ever. Decided to implement rather than remove the stepper (documented in implementation-decisions.md, "Post-migration review"): it is the one generator feature that serves this app's stated ROM-hack/draft-building audience, and SurpriseMeViewModel already loaded the custom roster into its UI state without using it. generateTeam and regenerateSlot now take a customs: List = emptyList() parameter (default keeps every pre-existing call site, all of them in tests, unchanged) and treat customSlots as a reserved category exactly like starter/legendary- mythical/Mega/Dynamax, via a small Candidate wrapper (entry-backed or custom) that unifies scoring and selection across both. One asymmetry, stated in the same doc entry: a custom is never chosen opportunistically in a free slot the way a catalogue Pokemon can be once its own quota is met, since customs live outside buildEligiblePool's catalogue-only pool. Setting customSlots = 0 must mean no custom ever appears. Adds six tests: generateTeam fills exactly N custom slots and never places one at N=0; regenerateSlot falls back to the custom roster when the real catalogue has nothing eligible, never introduces a custom at customSlots=0, and respects the cap already met by the other members. Full analysis: docs/post-migration-review.md, finding 5. Same verification caveat as the finding 1 commit: no Android SDK in this sandbox, so these tests are unverified by a local run; CI is watched. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_018zXVpxEbSWadqKyntXTXD6 --- CHANGELOG.md | 7 ++ .../domain/generator/TeamGenerator.kt | 104 ++++++++++++------ .../ui/surprise/SurpriseMeViewModel.kt | 4 +- .../marcogn/coverdex/domain/TestFixtures.kt | 3 +- .../domain/generator/TeamGeneratorTest.kt | 63 +++++++++++ docs/implementation-decisions.md | 42 +++++++ docs/test-plan.md | 13 +++ 7 files changed, 199 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5dcdda8..404b4d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,13 @@ versions follow [Semantic Versioning](https://semver.org/). enough — see finding 1 in [`docs/post-migration-review.md`](docs/post-migration-review.md). The score is now computed once per candidate and sorted on the stored value. +- **Surprise Me's "Custom slots" now actually places custom Pokémon.** + The stepper always consumed the six-slot budget but the generator never + read the constraint, so it silently placed none — see finding 5 in + [`docs/post-migration-review.md`](docs/post-migration-review.md). + Reserving N custom slots now fills exactly N slots from the saved + custom roster, the same "exactly N" semantics every other constraint + category already had. - **Post-migration review of the coverage and suggestion engines.** A code-level audit of the shipped app against the phase plans, diffing every Kotlin port against the TypeScript original recovered from git history. diff --git a/app/src/main/java/com/marcogn/coverdex/domain/generator/TeamGenerator.kt b/app/src/main/java/com/marcogn/coverdex/domain/generator/TeamGenerator.kt index 977b4ba..e01dfd9 100644 --- a/app/src/main/java/com/marcogn/coverdex/domain/generator/TeamGenerator.kt +++ b/app/src/main/java/com/marcogn/coverdex/domain/generator/TeamGenerator.kt @@ -14,11 +14,17 @@ import kotlin.random.Random * [Random] parameter, defaulting to [Random.Default] — rather than a direct `Math.random()` call, * so tests can seed it; see `docs/plan/phase-4-suggestions-and-generator.md` §3. * - * `buildEligiblePool`, `generateTeam` and `regenerateSlot` all drop the TypeScript's `customs: - * TeamMember[]` parameter: reading `teamGenerator.ts` end to end shows it is never referenced in - * any of the three function bodies (nor is `GeneratorConstraints.customSlots`, kept below only as - * a ported struct field) — a genuinely dead parameter in the original, not a behavioural - * difference. See `docs/implementation-decisions.md`, "Phase 4". + * `customs` is a deliberate native addition, not a port: `teamGenerator.ts` accepted a `customs: + * TeamMember[]` parameter but never referenced it in any of its three function bodies, and Phase 4 + * dropped it here as genuinely dead. `GeneratorConstraints.customSlots` was kept anyway as a ported + * struct field, which meant the Surprise Me screen shipped a "Custom slots" stepper that consumed + * the six-slot budget and placed nothing — see `docs/post-migration-review.md`, finding 5, and + * `docs/implementation-decisions.md`, "Post-migration review". `generateTeam` and `regenerateSlot` + * now take `customs` (default `emptyList()`, so every existing call site is unaffected) and honour + * `customSlots` as a reserved category exactly like starter/legendary-mythical/Mega/Dynamax, except + * a custom is never chosen opportunistically in a free slot the way a catalogue Pokémon can be + * once its own quota is met — customs live outside [buildEligiblePool]'s catalogue-only pool, so a + * custom appears only while its own reserved budget still has room. */ /** Hardcoded per-generation lists of starter final evolutions (Grass/Fire/Water), ported @@ -62,6 +68,15 @@ private fun isMegaOrDynamax(entry: PokemonEntry): Boolean = isMega(entry) || isD private fun findEntry(allPokemon: List, m: TeamMember): PokemonEntry? = allPokemon.find { it.displayName == m.speciesName || it.name == m.speciesName.lowercase() } +/** A generator candidate: either a catalogue entry (scored via [memberFromEntry], its ability + * taken from [PokemonEntry.defaultAbility]) or a custom roster [TeamMember] (its own ability kept + * as-is). [entry] is `null` for a custom — that is the single source of truth this file uses to + * tell the two apart, since a custom is never legendary/mythical, a starter, a Mega or a Dynamax. */ +private data class Candidate(val member: TeamMember, val entry: PokemonEntry?, val ability: String?) + +private fun candidateFromEntry(entry: PokemonEntry): Candidate = Candidate(memberFromEntry(entry), entry, entry.defaultAbility) +private fun candidateFromCustom(member: TeamMember): Candidate = Candidate(member, null, member.ability) + private fun currentTeamCoverage(chart: TypeChart, team: List): Set { val cov = mutableSetOf() team.forEach { cov.addAll(offensiveCoverageForMember(chart, it, false)) } @@ -111,6 +126,7 @@ fun generateTeam( lockedMembers: List, constraints: GeneratorConstraints, random: Random = Random.Default, + customs: List = emptyList(), ): GeneratorResult { val pool = buildEligiblePool(allPokemon, constraints) val slotsToFill = 6 - lockedMembers.size @@ -125,26 +141,31 @@ fun generateTeam( var starterCount = team.count { m -> findEntry(allPokemon, m)?.let { isStarter(it) } == true } var megaCount = team.count { m -> findEntry(allPokemon, m)?.let { isMega(it) } == true } var dynamaxCount = team.count { m -> findEntry(allPokemon, m)?.let { isDynamax(it) } == true } + var customCount = team.count { it.isCustomSaved } var starterSlotsRemaining = maxOf(0, constraints.starterSlots - starterCount) var legendaryMythicalSlotsRemaining = maxOf(0, constraints.legendaryMythicalSlots - legendaryMythicalCount) var megaSlotsRemaining = maxOf(0, constraints.megaSlots - megaCount) var dynamaxSlotsRemaining = maxOf(0, constraints.dynamaxSlots - dynamaxCount) + var customSlotsRemaining = maxOf(0, constraints.customSlots - customCount) repeat(slotsToFill) { - var candidatePool: List + val candidates: List if (legendaryMythicalSlotsRemaining > 0) { - candidatePool = pool.filter { isLegendaryOrMythical(it) && it.displayName.lowercase() !in usedSpecies } + candidates = pool.filter { isLegendaryOrMythical(it) && it.displayName.lowercase() !in usedSpecies }.map(::candidateFromEntry) legendaryMythicalSlotsRemaining-- } else if (starterSlotsRemaining > 0) { - candidatePool = pool.filter { isStarter(it) && it.displayName.lowercase() !in usedSpecies } + candidates = pool.filter { isStarter(it) && it.displayName.lowercase() !in usedSpecies }.map(::candidateFromEntry) starterSlotsRemaining-- + } else if (customSlotsRemaining > 0) { + candidates = customs.filter { it.speciesName.lowercase() !in usedSpecies }.map(::candidateFromCustom) + customSlotsRemaining-- } else if (megaSlotsRemaining > 0) { - candidatePool = pool.filter { isMega(it) && it.displayName.lowercase() !in usedSpecies } + candidates = pool.filter { isMega(it) && it.displayName.lowercase() !in usedSpecies }.map(::candidateFromEntry) megaSlotsRemaining-- } else if (dynamaxSlotsRemaining > 0) { - candidatePool = pool.filter { isDynamax(it) && it.displayName.lowercase() !in usedSpecies } + candidates = pool.filter { isDynamax(it) && it.displayName.lowercase() !in usedSpecies }.map(::candidateFromEntry) dynamaxSlotsRemaining-- } else { var free = pool.filter { it.displayName.lowercase() !in usedSpecies } @@ -160,25 +181,29 @@ fun generateTeam( if (constraints.dynamaxSlots > 0 && dynamaxCount >= constraints.dynamaxSlots) { free = free.filterNot { isDynamax(it) } } - candidatePool = free + candidates = free.map(::candidateFromEntry) } - if (candidatePool.isEmpty()) { + if (candidates.isEmpty()) { return GeneratorResult(team = team, warning = "tooFewPokemon") } - val best = candidatePool - .map { entry -> entry to memberFromEntry(entry) } - .maxByOrNull { (_, member) -> computeScore(chart, member, team, random) }!! + // maxByOrNull calls its selector exactly once per element (never per comparison), so this + // is safe even though computeScore adds fresh random noise per call — see regenerateSlot's + // own note below on the sort that must not do the same thing the same way. + val best = candidates.maxByOrNull { candidate -> computeScore(chart, candidate.member, team, random) }!! - val newMember = best.second.copy(ability = best.first.defaultAbility) - team.add(newMember) - usedSpecies.add(best.first.displayName.lowercase()) + team.add(best.member.copy(ability = best.ability)) + usedSpecies.add(best.member.speciesName.lowercase()) - if (isLegendaryOrMythical(best.first)) legendaryMythicalCount++ - if (isStarter(best.first)) starterCount++ - if (isMega(best.first)) megaCount++ - if (isDynamax(best.first)) dynamaxCount++ + if (best.entry != null) { + if (isLegendaryOrMythical(best.entry)) legendaryMythicalCount++ + if (isStarter(best.entry)) starterCount++ + if (isMega(best.entry)) megaCount++ + if (isDynamax(best.entry)) dynamaxCount++ + } else { + customCount++ + } } return GeneratorResult(team = team) @@ -195,30 +220,44 @@ fun regenerateSlot( slotIndex: Int, constraints: GeneratorConstraints, random: Random = Random.Default, + customs: List = emptyList(), ): TeamMember { val otherMembers = currentTeam.filterIndexed { i, _ -> i != slotIndex } val pool = buildEligiblePool(allPokemon, constraints) val usedSpecies = otherMembers.mapTo(mutableSetOf()) { it.speciesName.lowercase() } - var candidatePool = pool.filter { it.displayName.lowercase() !in usedSpecies } + var entryPool = pool.filter { it.displayName.lowercase() !in usedSpecies } if (constraints.legendaryMythicalSlots > 0) { val count = otherMembers.count { m -> findEntry(allPokemon, m)?.let { isLegendaryOrMythical(it) } == true } - if (count >= constraints.legendaryMythicalSlots) candidatePool = candidatePool.filterNot { isLegendaryOrMythical(it) } + if (count >= constraints.legendaryMythicalSlots) entryPool = entryPool.filterNot { isLegendaryOrMythical(it) } } if (constraints.starterSlots > 0) { val count = otherMembers.count { m -> findEntry(allPokemon, m)?.let { isStarter(it) } == true } - if (count >= constraints.starterSlots) candidatePool = candidatePool.filterNot { isStarter(it) } + if (count >= constraints.starterSlots) entryPool = entryPool.filterNot { isStarter(it) } } if (constraints.megaSlots > 0) { val count = otherMembers.count { m -> findEntry(allPokemon, m)?.let { isMega(it) } == true } - if (count >= constraints.megaSlots) candidatePool = candidatePool.filterNot { isMega(it) } + if (count >= constraints.megaSlots) entryPool = entryPool.filterNot { isMega(it) } } if (constraints.dynamaxSlots > 0) { val count = otherMembers.count { m -> findEntry(allPokemon, m)?.let { isDynamax(it) } == true } - if (count >= constraints.dynamaxSlots) candidatePool = candidatePool.filterNot { isDynamax(it) } + if (count >= constraints.dynamaxSlots) entryPool = entryPool.filterNot { isDynamax(it) } } + // A custom is a regeneration candidate only while its own reserved budget still has room — + // same rule generateTeam applies, since customs sit outside buildEligiblePool's catalogue-only + // pool and are never picked opportunistically the way a catalogue Pokémon can be once its + // quota is met (see the class doc above). + val customPool: List = if (constraints.customSlots > 0) { + val count = otherMembers.count { it.isCustomSaved } + if (count >= constraints.customSlots) emptyList() else customs.filter { it.speciesName.lowercase() !in usedSpecies } + } else { + emptyList() + } + + val candidatePool = entryPool.map(::candidateFromEntry) + customPool.map(::candidateFromCustom) + if (candidatePool.isEmpty()) { return currentTeam[slotIndex] } @@ -229,14 +268,11 @@ fun regenerateSlot( // Collections.sort's TimSort throw once the pool is large enough to leave insertion-sort // territory (candidatePool is the full eligible catalogue here, not a test-sized fixture). val scored = candidatePool - .map { entry -> - val member = memberFromEntry(entry) - Triple(entry, member, computeScore(chart, member, otherMembers, random)) - } - .sortedByDescending { (_, _, score) -> score } + .map { candidate -> candidate to computeScore(chart, candidate.member, otherMembers, random) } + .sortedByDescending { (_, score) -> score } val topN = minOf(5, scored.size) - val picked = scored[random.nextInt(topN)] + val picked = scored[random.nextInt(topN)].first - return picked.second.copy(ability = picked.first.defaultAbility) + return picked.member.copy(ability = picked.ability) } diff --git a/app/src/main/java/com/marcogn/coverdex/ui/surprise/SurpriseMeViewModel.kt b/app/src/main/java/com/marcogn/coverdex/ui/surprise/SurpriseMeViewModel.kt index 591574b..2d77a0a 100644 --- a/app/src/main/java/com/marcogn/coverdex/ui/surprise/SurpriseMeViewModel.kt +++ b/app/src/main/java/com/marcogn/coverdex/ui/surprise/SurpriseMeViewModel.kt @@ -99,7 +99,7 @@ class SurpriseMeViewModel @Inject constructor( fun generate() { val chart = uiState.value.chart ?: return - val res = generateTeam(chart, uiState.value.pool, lockedMembers.value, constraints.value) + val res = generateTeam(chart, uiState.value.pool, lockedMembers.value, constraints.value, customs = uiState.value.customs) result.value = res.team warning.value = res.warning } @@ -110,7 +110,7 @@ class SurpriseMeViewModel @Inject constructor( val chart = uiState.value.chart ?: return val current = result.value if (index !in current.indices) return - val newMember = regenerateSlot(chart, uiState.value.pool, current, index, constraints.value) + val newMember = regenerateSlot(chart, uiState.value.pool, current, index, constraints.value, customs = uiState.value.customs) result.value = current.toMutableList().also { it[index] = newMember } } diff --git a/app/src/test/java/com/marcogn/coverdex/domain/TestFixtures.kt b/app/src/test/java/com/marcogn/coverdex/domain/TestFixtures.kt index ae19343..31dc80c 100644 --- a/app/src/test/java/com/marcogn/coverdex/domain/TestFixtures.kt +++ b/app/src/test/java/com/marcogn/coverdex/domain/TestFixtures.kt @@ -176,6 +176,7 @@ fun buildMember( moveTypes: List = emptyList(), id: String = "member-$speciesName", ability: String? = null, + isCustomSaved: Boolean = false, ): TeamMember { val moves = MutableList(4) { null } moveTypes.take(4).forEachIndexed { i, mt -> @@ -195,7 +196,7 @@ fun buildMember( types = types, ability = ability, moves = moves, - isCustomSaved = false, + isCustomSaved = isCustomSaved, ) } diff --git a/app/src/test/java/com/marcogn/coverdex/domain/generator/TeamGeneratorTest.kt b/app/src/test/java/com/marcogn/coverdex/domain/generator/TeamGeneratorTest.kt index 81ea2a6..60da516 100644 --- a/app/src/test/java/com/marcogn/coverdex/domain/generator/TeamGeneratorTest.kt +++ b/app/src/test/java/com/marcogn/coverdex/domain/generator/TeamGeneratorTest.kt @@ -88,6 +88,31 @@ class TeamGeneratorTest { } } + // ---- custom slots ---- + + private val customRoster = listOf( + buildMember("CustomA", PokemonType.GHOST to null, isCustomSaved = true), + buildMember("CustomB", PokemonType.DRAGON to null, isCustomSaved = true), + buildMember("CustomC", PokemonType.FAIRY to null, isCustomSaved = true), + ) + + @Test + fun `generateTeam fills exactly customSlots slots from the roster, before any catalogue candidate`() { + val constraints = DEFAULT_CONSTRAINTS.copy(customSlots = 2) + val result = generateTeam(chart, pool, emptyList(), constraints, Random(1), customs = customRoster) + assertEquals(6, result.team.size) + assertEquals(2, result.team.count { it.isCustomSaved }) + // legendaryMythicalSlots/starterSlots are both 0 by default, so the reserved custom + // category is the first branch generateTeam's per-slot loop actually takes. + assertTrue(result.team.take(2).all { it.isCustomSaved }) + } + + @Test + fun `generateTeam never places a custom Pokemon when customSlots is 0`() { + val result = generateTeam(chart, pool, emptyList(), DEFAULT_CONSTRAINTS, Random(1), customs = customRoster) + assertFalse(result.team.any { it.isCustomSaved }) + } + // ---- buildEligiblePool ---- @Test @@ -130,6 +155,44 @@ class TeamGeneratorTest { } } + @Test + fun `regenerateSlot falls back to the custom roster when the real pool has nothing eligible`() { + val team = sixMemberTeam() + val customs = listOf(buildMember("CustomMon", PokemonType.GHOST to null, isCustomSaved = true)) + val constraints = DEFAULT_CONSTRAINTS.copy(customSlots = 1) + // An empty catalogue means buildEligiblePool always returns emptyList(), so the only + // possible candidate is the custom below — this isolates the merge of entryPool and + // customPool from the scoring competition between the two. + val newMember = regenerateSlot(chart, emptyList(), team, 5, constraints, Random(1), customs = customs) + assertEquals("CustomMon", newMember.speciesName) + assertTrue(newMember.isCustomSaved) + } + + @Test + fun `regenerateSlot never introduces a custom Pokemon when customSlots is 0`() { + val team = sixMemberTeam() + val customs = listOf(buildMember("CustomMon", PokemonType.GHOST to null, isCustomSaved = true)) + for (seed in 0..9) { + val newMember = regenerateSlot(chart, pool, team, 5, DEFAULT_CONSTRAINTS, Random(seed), customs = customs) + assertFalse(newMember.isCustomSaved) + } + } + + @Test + fun `regenerateSlot respects the customSlots cap already met by the other members`() { + val customA = buildMember("CustomMon", PokemonType.GHOST to null, isCustomSaved = true) + val customB = buildMember("CustomMon2", PokemonType.DRAGON to null, isCustomSaved = true) + val team = sixMemberTeam().mapIndexed { i, m -> if (i == 0) customA else m } + val constraints = DEFAULT_CONSTRAINTS.copy(customSlots = 1) + // otherMembers (every slot except 5) already includes customA, meeting the customSlots=1 + // cap, so customB must never be offered as a candidate for slot 5 — even with a real, + // non-empty catalogue pool competing for the slot alongside it. + for (seed in 0..9) { + val newMember = regenerateSlot(chart, pool, team, 5, constraints, Random(seed), customs = listOf(customB)) + assertFalse(newMember.speciesName == "CustomMon2") + } + } + // A same-typed synthetic pool gives every candidate an identical base composite score, so // only computeScore's random noise breaks ties — the worst case for a comparator that must // stay consistent across repeated evaluations. Before the fix, sortedByDescending re-rolled diff --git a/docs/implementation-decisions.md b/docs/implementation-decisions.md index 00a730d..4fe5c8d 100644 --- a/docs/implementation-decisions.md +++ b/docs/implementation-decisions.md @@ -701,3 +701,45 @@ the way this session briefly did: the two purely positive "it's native now" bullets — stated plainly rather than softened, per `docs/plan/native-spec.md`'s own instruction that "Phase 6's release notes must lead with that warning." + +## Post-migration review + +Findings from `docs/post-migration-review.md`, a code-level audit run +after Phase 6 closed. Each finding below records the decision made when +fixing it; the review document itself has the full analysis, including +findings not yet acted on. + +- **Finding 1 (crash) — fixed as a pure bug fix, no decision needed.** + `regenerateSlot`'s ranking recomputed its composite score (including + random tie-breaking noise) inside the sort comparator instead of once + per candidate, which could throw `IllegalArgumentException` once the + eligible pool was large. The production pool is in the high hundreds; + every existing test pool was under 20 entries, which is why nothing + caught it. Fixed by scoring once per candidate before sorting. +- **Finding 5 (Surprise Me's "Custom slots" stepper did nothing) — + implemented rather than removed.** `teamGenerator.ts` accepted a + `customs: TeamMember[]` parameter it never referenced, and Phase 4's + port dropped it as genuinely dead — but kept + `GeneratorConstraints.customSlots` as a ported struct field, so the + Surprise Me screen shipped a stepper that consumed the six-slot budget + and placed nothing. Decided to implement rather than delete the + stepper: it is the one generator feature that serves this app's stated + ROM-hack/draft-building audience (`CLAUDE.md`, "What this project + is"), and `SurpriseMeViewModel` already loaded the custom roster into + its UI state without using it. `generateTeam` and `regenerateSlot` now + take a `customs: List = emptyList()` parameter (default + keeps every pre-existing call site, including every test, unchanged) + and treat `customSlots` as a reserved category exactly like starter/ + legendary-mythical/Mega/Dynamax — with one asymmetry, stated here + because nothing forced it either way: a custom is **never** chosen + opportunistically in an unconstrained "free" slot the way a catalogue + Pokémon can be once its own quota is met, because customs live outside + `buildEligiblePool`'s catalogue-only pool. A custom appears only while + its own reserved budget still has room. The alternative (customs + competing for every free slot too) was rejected as surprising: a user + who sets `customSlots = 0` should never see a custom Pokémon appear. +- **Finding 6 (abilities ignored by suggestion/generator scoring) — not + yet acted on.** Left for its own follow-up commit since it changes the + composite score's output, not just its performance or a missing + feature — it needs updated test expectations alongside it, not bundled + with an unrelated fix. diff --git a/docs/test-plan.md b/docs/test-plan.md index 823e4d4..e6390dd 100644 --- a/docs/test-plan.md +++ b/docs/test-plan.md @@ -207,6 +207,11 @@ None yet. legendary-mythical/Mega/Dynamax counters (each capped so the total plus locked anchors never exceeds 6); Generate produces a full team, anchors first and unchanged. +- [ ] **Surprise Me — custom slots actually place custom Pokémon.** With + at least one saved custom roster entry, the "Custom" counter appears; + setting it to N and generating yields exactly N custom Pokémon in the + result (previously it silently placed none while still consuming the + six-slot budget — see Known regressions below). - [ ] **Surprise Me — regenerate.** Regenerating a single (non-anchor) slot changes only that slot; "Regenerate all" produces an entirely new team respecting the same anchors and constraints. @@ -226,6 +231,14 @@ None yet. testing; reproduced with a standalone JVM harness before the fix, not on a device. `regenerateSlot`'s candidate ranking now scores each candidate once instead of re-rolling the random tie-breaker on every comparison. +- **Fixed 2026-09-05.** Surprise Me's "Custom slots" stepper consumed the + six-slot budget (`remainingSlots`, `budgetFull`) but never placed a + single custom Pokémon — `TeamGenerator.kt` never read + `GeneratorConstraints.customSlots` — found by code review + (`docs/post-migration-review.md`, finding 5), not by manual testing. + `generateTeam` and `regenerateSlot` now treat `customSlots` as a + reserved category, filled from the custom roster already loaded into + `SurpriseMeViewModel`'s UI state. ## Phase 5 — Showdown import/export, settings and local backup From e9796d2516ae4234f8e89a9972e7f2e8a3faae4e Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 07:22:21 +0000 Subject: [PATCH 4/8] Move coverage/suggestion computation off the main thread (finding 2) AnalysisViewModel's combine() chain (analyseTeam, computeSuggestions, sharedWeaknessCounts) and both of SurpriseMeViewModel's generator entry points ran on Dispatchers.Main.immediate: the former inside stateIn's own collector, the latter as plain non-suspend functions called directly from Compose click handlers. Both do real work against a catalogue in the high hundreds of entries, recomputed on every team edit, toggle flip, filter change, and generation tap. AnalysisViewModel: the final combine() now ends in .flowOn(Dispatchers.Default).stateIn(...), moving the whole transform lambda off Main. SurpriseMeViewModel: generate()/regenerateSlot() set isGenerating.value = true synchronously (before launching), then run the entire computation and every result/warning/isGenerating write inside viewModelScope.launch(Dispatchers.Default) { ... } - one hop to a real background thread, no hop back through Main, since MutableStateFlow.value can be set from any thread. Documented in implementation-decisions.md why this shape was chosen over withContext(Dispatchers.Default) { compute() } with the writes left on the calling context, and why no injectable test dispatcher was added (Dispatchers.Default is hardcoded, same convention as the direct Dispatchers.IO calls already in data/pokeapi and data/backup). SurpriseMeScreen gained a progress indicator on the Generate button and disables every generation action (Generate, Regenerate slot, Regenerate all, Keep) while one is in flight. Updated the one existing test whose comment assumed regenerateSlot was synchronous - the state was already non-empty from a prior generate() call in the same test, so waiting on result.isNotEmpty() alone could match stale data; now waits for !isGenerating too, which cannot, since isGenerating flips to true synchronously before the async work even starts. Added a test asserting isGenerating's true/false transition, and one on AnalysisViewModel confirming coverage completes without the test's Main dispatcher ever being advanced. Full analysis: docs/post-migration-review.md, finding 2. Same verification caveat as prior commits in this series: no Android SDK in this sandbox, so these tests are unverified by a local run; CI is watched. The reasoning about which Flow-based waits are safe under StandardTestDispatcher (and which existing test patterns already prove it) is laid out in the design decision recorded in implementation-decisions.md, but it is reasoning, not a local test run. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_018zXVpxEbSWadqKyntXTXD6 --- CHANGELOG.md | 9 ++++ .../coverdex/ui/surprise/SurpriseMeScreen.kt | 25 ++++++++-- .../coverdex/ui/surprise/SurpriseMeUiState.kt | 6 ++- .../ui/surprise/SurpriseMeViewModel.kt | 50 +++++++++++++++---- .../ui/team/analysis/AnalysisViewModel.kt | 19 +++++-- .../ui/surprise/SurpriseMeViewModelTest.kt | 27 ++++++++-- .../ui/team/analysis/AnalysisViewModelTest.kt | 15 ++++++ docs/implementation-decisions.md | 24 +++++++++ docs/test-plan.md | 13 +++++ 9 files changed, 164 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 404b4d9..895a6f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,15 @@ versions follow [Semantic Versioning](https://semver.org/). Reserving N custom slots now fills exactly N slots from the saved custom roster, the same "exactly N" semantics every other constraint category already had. +- **Coverage analysis and team generation no longer run on the main + thread.** Both `AnalysisScreen`'s coverage/suggestions pipeline and + Surprise Me's generator scored every eligible candidate against the + whole team on `Dispatchers.Main.immediate` — real work against a + catalogue in the high hundreds, not the handful of entries any unit + test's pool has. See finding 2 in + [`docs/post-migration-review.md`](docs/post-migration-review.md). + Surprise Me now shows a progress indicator and disables its + Generate/Regenerate actions while a generation is in flight. - **Post-migration review of the coverage and suggestion engines.** A code-level audit of the shipped app against the phase plans, diffing every Kotlin port against the TypeScript original recovered from git history. diff --git a/app/src/main/java/com/marcogn/coverdex/ui/surprise/SurpriseMeScreen.kt b/app/src/main/java/com/marcogn/coverdex/ui/surprise/SurpriseMeScreen.kt index 9709674..9b50575 100644 --- a/app/src/main/java/com/marcogn/coverdex/ui/surprise/SurpriseMeScreen.kt +++ b/app/src/main/java/com/marcogn/coverdex/ui/surprise/SurpriseMeScreen.kt @@ -19,6 +19,7 @@ import androidx.compose.material.icons.filled.Casino import androidx.compose.material.icons.filled.Close import androidx.compose.material3.Button import androidx.compose.material3.Card +import androidx.compose.material3.CircularProgressIndicator import androidx.compose.material3.FilterChip import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme @@ -162,7 +163,15 @@ fun SurpriseMeScreen( } Button(onClick = { viewModel.generate() }, enabled = state.canGenerate, modifier = Modifier.fillMaxWidth()) { - Text(stringResource(R.string.surprise_me_generate)) + if (state.isGenerating) { + CircularProgressIndicator( + modifier = Modifier.size(20.dp), + strokeWidth = 2.dp, + color = MaterialTheme.colorScheme.onPrimary, + ) + } else { + Text(stringResource(R.string.surprise_me_generate)) + } } // Result. @@ -190,16 +199,24 @@ fun SurpriseMeScreen( pokedexId = member.pokedexId, types = member.types, ability = member.ability, - canRegenerate = !isLocked, + canRegenerate = !isLocked && !state.isGenerating, onRegenerate = { viewModel.regenerateSlot(index) }, ) } } Row(horizontalArrangement = Arrangement.spacedBy(8.dp)) { - OutlinedButton(onClick = { viewModel.regenerateAll() }, modifier = Modifier.weight(1f)) { + OutlinedButton( + onClick = { viewModel.regenerateAll() }, + enabled = !state.isGenerating, + modifier = Modifier.weight(1f), + ) { Text(stringResource(R.string.surprise_me_regenerate_all)) } - Button(onClick = { showKeepDialog = true }, modifier = Modifier.weight(1f)) { + Button( + onClick = { showKeepDialog = true }, + enabled = !state.isGenerating, + modifier = Modifier.weight(1f), + ) { Text(stringResource(R.string.surprise_me_keep)) } } diff --git a/app/src/main/java/com/marcogn/coverdex/ui/surprise/SurpriseMeUiState.kt b/app/src/main/java/com/marcogn/coverdex/ui/surprise/SurpriseMeUiState.kt index ede4525..bbd8402 100644 --- a/app/src/main/java/com/marcogn/coverdex/ui/surprise/SurpriseMeUiState.kt +++ b/app/src/main/java/com/marcogn/coverdex/ui/surprise/SurpriseMeUiState.kt @@ -19,10 +19,14 @@ data class SurpriseMeUiState( /** `"tooFewPokemon"` when the pool couldn't fill every slot — resolved to a string resource * by the screen, matching `GeneratorResult.warning`'s own string-key convention. */ val warning: String? = null, + /** True while `generate()`/`regenerateSlot()` are running on [kotlinx.coroutines.Dispatchers.Default] + * — see `docs/post-migration-review.md`, finding 2. Drives the Generate button's spinner and + * disables every regenerate action so a second tap can't race the one in flight. */ + val isGenerating: Boolean = false, ) { val anchorCount: Int get() = lockedMembers.size val constraintTotal: Int get() = with(constraints) { starterSlots + legendaryMythicalSlots + megaSlots + dynamaxSlots + customSlots } val remainingSlots: Int get() = (6 - anchorCount - constraintTotal).coerceAtLeast(0) val budgetFull: Boolean get() = anchorCount + constraintTotal >= 6 - val canGenerate: Boolean get() = chart != null + val canGenerate: Boolean get() = chart != null && !isGenerating } diff --git a/app/src/main/java/com/marcogn/coverdex/ui/surprise/SurpriseMeViewModel.kt b/app/src/main/java/com/marcogn/coverdex/ui/surprise/SurpriseMeViewModel.kt index 2d77a0a..a755db6 100644 --- a/app/src/main/java/com/marcogn/coverdex/ui/surprise/SurpriseMeViewModel.kt +++ b/app/src/main/java/com/marcogn/coverdex/ui/surprise/SurpriseMeViewModel.kt @@ -16,6 +16,7 @@ import com.marcogn.coverdex.domain.repository.TeamRepository import dagger.hilt.android.lifecycle.HiltViewModel import java.util.UUID import javax.inject.Inject +import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted @@ -43,30 +44,40 @@ class SurpriseMeViewModel @Inject constructor( private val constraints = MutableStateFlow(DEFAULT_CONSTRAINTS) private val result = MutableStateFlow>(emptyList()) private val warning = MutableStateFlow(null) + private val isGenerating = MutableStateFlow(false) private data class Core(val chart: TypeChart?, val pool: List, val customs: List) + /** [result], [warning] and [isGenerating] grouped so the final `combine()` below stays within + * the stdlib's typed 5-flow overload — same reason `AnalysisViewModel` groups its own flows + * into an intermediate data class rather than reaching for the untyped vararg overload. */ + private data class GenerationState(val result: List, val warning: String?, val isGenerating: Boolean) + private val core: Flow = combine( pokedexRepository.cacheStatus.map { status -> if (status.isUsable) pokedexRepository.typeChart() else null }, pokedexRepository.cacheStatus.map { status -> if (status.isUsable) pokedexRepository.allSpecies() else emptyList() }, customPokemonRepository.roster, ) { chart, pool, roster -> Core(chart, pool, roster) } + private val generationState: Flow = combine(result, warning, isGenerating) { res, warn, generating -> + GenerationState(res, warn, generating) + } + val uiState: StateFlow = combine( core, lockedMembers, constraints, - result, - warning, - ) { core, locked, cons, res, warn -> + generationState, + ) { core, locked, cons, gen -> SurpriseMeUiState( chart = core.chart, pool = core.pool, customs = core.customs, lockedMembers = locked, constraints = cons, - result = res, - warning = warn, + result = gen.result, + warning = gen.warning, + isGenerating = gen.isGenerating, ) }.stateIn( scope = viewModelScope, @@ -97,21 +108,40 @@ class SurpriseMeViewModel @Inject constructor( constraints.value = transform(constraints.value) } + /** `generateTeam`/`regenerateSlot` (below) score every eligible candidate against the whole + * team on every step — real work against a catalogue in the high hundreds, not the handful of + * entries any unit test's pool has. Launched directly on [Dispatchers.Default] (not via + * `withContext` from a `Dispatchers.Main` coroutine) so the whole computation, including the + * final [result]/[warning]/[isGenerating] writes, runs on a real background thread with no + * hop back through the main dispatcher — see `docs/post-migration-review.md`, finding 2. + * [isGenerating] itself flips to `true` synchronously, before the coroutine is even launched, + * so a caller — a Compose click handler, or a test's `uiState.first { !it.isGenerating }` — + * always observes the state change immediately rather than racing it. */ fun generate() { + if (isGenerating.value) return val chart = uiState.value.chart ?: return - val res = generateTeam(chart, uiState.value.pool, lockedMembers.value, constraints.value, customs = uiState.value.customs) - result.value = res.team - warning.value = res.warning + isGenerating.value = true + viewModelScope.launch(Dispatchers.Default) { + val res = generateTeam(chart, uiState.value.pool, lockedMembers.value, constraints.value, customs = uiState.value.customs) + result.value = res.team + warning.value = res.warning + isGenerating.value = false + } } fun regenerateAll() = generate() fun regenerateSlot(index: Int) { + if (isGenerating.value) return val chart = uiState.value.chart ?: return val current = result.value if (index !in current.indices) return - val newMember = regenerateSlot(chart, uiState.value.pool, current, index, constraints.value, customs = uiState.value.customs) - result.value = current.toMutableList().also { it[index] = newMember } + isGenerating.value = true + viewModelScope.launch(Dispatchers.Default) { + val newMember = regenerateSlot(chart, uiState.value.pool, current, index, constraints.value, customs = uiState.value.customs) + result.value = current.toMutableList().also { it[index] = newMember } + isGenerating.value = false + } } /** Creates a brand-new team named [teamName] and writes the generated result into its six diff --git a/app/src/main/java/com/marcogn/coverdex/ui/team/analysis/AnalysisViewModel.kt b/app/src/main/java/com/marcogn/coverdex/ui/team/analysis/AnalysisViewModel.kt index 1b75faa..afd0b97 100644 --- a/app/src/main/java/com/marcogn/coverdex/ui/team/analysis/AnalysisViewModel.kt +++ b/app/src/main/java/com/marcogn/coverdex/ui/team/analysis/AnalysisViewModel.kt @@ -21,11 +21,13 @@ import com.marcogn.coverdex.ui.navigation.Destination import dagger.hilt.android.lifecycle.HiltViewModel import java.util.UUID import javax.inject.Inject +import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.flowOn import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch @@ -127,11 +129,18 @@ class AnalysisViewModel @Inject constructor( includeCustomsAnalysis = includeCustoms, generationFilter = genFilter, ) - }.stateIn( - scope = viewModelScope, - started = SharingStarted.WhileSubscribed(5_000), - initialValue = AnalysisUiState(), - ) + } + // analyseTeam/computeSuggestions/sharedWeaknessCounts are real work against a catalogue + // in the high hundreds (docs/post-migration-review.md, finding 2) — flowOn moves the + // whole combine() above, transform lambda included, off Dispatchers.Main.immediate + // (what viewModelScope.launch inside stateIn would otherwise use) and onto a background + // thread, recomputed on every team edit, toggle flip and filter change alike. + .flowOn(Dispatchers.Default) + .stateIn( + scope = viewModelScope, + started = SharingStarted.WhileSubscribed(5_000), + initialValue = AnalysisUiState(), + ) fun setIncludeCustomsAnalysis(value: Boolean) { viewModelScope.launch { settingsPreferences.setIncludeCustomsAnalysis(value) } diff --git a/app/src/test/java/com/marcogn/coverdex/ui/surprise/SurpriseMeViewModelTest.kt b/app/src/test/java/com/marcogn/coverdex/ui/surprise/SurpriseMeViewModelTest.kt index c19b7ad..42920cb 100644 --- a/app/src/test/java/com/marcogn/coverdex/ui/surprise/SurpriseMeViewModelTest.kt +++ b/app/src/test/java/com/marcogn/coverdex/ui/surprise/SurpriseMeViewModelTest.kt @@ -88,6 +88,20 @@ class SurpriseMeViewModelTest { assertTrue(state.result.size <= 6) } + @Test + fun `isGenerating is true immediately after generate() and false again once it completes`() = runTest(mainDispatcherRule.dispatcher) { + val vm = viewModel() + vm.uiState.first { it.chart != null } + + vm.generate() + // isGenerating flips synchronously, before generate() even returns — no suspension + // needed to observe it, unlike the background computation itself. + assertTrue(vm.uiState.value.isGenerating) + + val state = vm.uiState.first { !it.isGenerating } + assertTrue(state.result.isNotEmpty()) + } + @Test fun `regenerateSlot only changes the targeted non-locked slot`() = runTest(mainDispatcherRule.dispatcher) { val vm = viewModel() @@ -97,10 +111,15 @@ class SurpriseMeViewModelTest { vm.regenerateSlot(0) - // regenerateSlot is synchronous (no coroutine hop), so the very next emission already - // reflects it — asserting inequality against `before` would hang if the (unseeded) - // generator happens to reselect slot 0's own prior occupant, which is legal. - val after = vm.uiState.first { it.result.isNotEmpty() }.result + // regenerateSlot now runs on Dispatchers.Default (docs/post-migration-review.md, finding + // 2), so the update is no longer synchronous. isGenerating flips to true synchronously, + // before regenerateSlot() even returns, and back to false only once the background work + // finishes and writes the new result — waiting for it to go false again cannot match the + // pre-regeneration state the way waiting on result.isNotEmpty() alone could (that + // predicate was already true from the generate() call above). Asserting inequality + // against `before` would still be wrong: the (unseeded) generator can legally reselect + // slot 0's own prior occupant. + val after = vm.uiState.first { !it.isGenerating && it.result.isNotEmpty() }.result assertEquals(before.size, after.size) for (i in 1 until before.size) { assertEquals(before[i].speciesName, after[i].speciesName) diff --git a/app/src/test/java/com/marcogn/coverdex/ui/team/analysis/AnalysisViewModelTest.kt b/app/src/test/java/com/marcogn/coverdex/ui/team/analysis/AnalysisViewModelTest.kt index dc51376..5616039 100644 --- a/app/src/test/java/com/marcogn/coverdex/ui/team/analysis/AnalysisViewModelTest.kt +++ b/app/src/test/java/com/marcogn/coverdex/ui/team/analysis/AnalysisViewModelTest.kt @@ -145,6 +145,21 @@ class AnalysisViewModelTest { assertEquals(listOf("Garchomp"), gen4.suggestions.map { it.candidateLabel }) } + @Test + fun `coverage is computed without ever advancing the Main test dispatcher`() = runTest(mainDispatcherRule.dispatcher) { + // mainDispatcherRule.dispatcher is a StandardTestDispatcher, which only runs work queued + // on it when explicitly advanced (advanceUntilIdle()/runCurrent()) — this test never + // calls either. Before finding 2's fix, analyseTeam ran inside stateIn's own collector, + // i.e. on Dispatchers.Main.immediate, so this would hang until timing out; flowOn(Default) + // moves it onto a real background thread instead, which completes independently. + settingsPreferences.setShowMoves(false) + val teamId = teamRepository.createTeam("T-dispatcher") + teamRepository.saveMember(teamId, 0, waterFlyingWithElectricMove()) + + val state = viewModel(teamId).uiState.first { it.coverage != null } + assertTrue(state.coverage!!.unionCovered.isNotEmpty()) + } + @Test fun `applySuggestion in addition mode writes the candidate into the first empty slot`() = runTest(mainDispatcherRule.dispatcher) { settingsPreferences.setShowMoves(false) diff --git a/docs/implementation-decisions.md b/docs/implementation-decisions.md index 4fe5c8d..b8012ea 100644 --- a/docs/implementation-decisions.md +++ b/docs/implementation-decisions.md @@ -738,6 +738,30 @@ findings not yet acted on. its own reserved budget still has room. The alternative (customs competing for every free slot too) was rejected as surprising: a user who sets `customSlots = 0` should never see a custom Pokémon appear. +- **Finding 2 (both engines ran on the main thread) — no injected test + dispatcher.** `AnalysisViewModel`'s `combine()` chain now ends in + `.flowOn(Dispatchers.Default).stateIn(...)`; `SurpriseMeViewModel. + generate()`/`regenerateSlot()` now do `isGenerating.value = true` + synchronously, then `viewModelScope.launch(Dispatchers.Default) { ... + }` for the whole computation and every `result`/`warning`/ + `isGenerating` write — deliberately not `withContext(Dispatchers. + Default) { compute() }` followed by writes back on the launch's + original (Main) context, which would need a second hop back through + `Dispatchers.Main` after the background work finishes. Considered + adding an injectable `@DefaultDispatcher` qualifier to + `CoroutinesModule.kt` (the established pattern for cross-cutting + coroutine concerns here, see `@ApplicationScope`) so tests could + substitute a deterministic `TestDispatcher`; decided against it for + now; `Dispatchers.Default` is hardcoded, matching this codebase's + existing convention of calling `Dispatchers.IO` directly in + `data/pokeapi`/`data/backup` rather than injecting a dispatcher there + either. `SurpriseMeViewModel`'s synchronous `isGenerating.value = true` + before the launch is why this is safe to test without one: a test can + assert on `vm.uiState.value.isGenerating` immediately after calling + `generate()`, with no suspension needed, and the eventual `false` is + observed via a real `StateFlow` update from a real background thread — + not virtual time — the same category of wait every Room-/DataStore- + backed test in this codebase already relies on. - **Finding 6 (abilities ignored by suggestion/generator scoring) — not yet acted on.** Left for its own follow-up commit since it changes the composite score's output, not just its performance or a missing diff --git a/docs/test-plan.md b/docs/test-plan.md index e6390dd..1b9c973 100644 --- a/docs/test-plan.md +++ b/docs/test-plan.md @@ -221,6 +221,11 @@ None yet. - [ ] **Surprise Me — Keep.** Tapping Keep prompts for a team name, then creates a brand-new team with the generated six slots and navigates to it — the Teams list shows it immediately. +- [ ] **Surprise Me — generating shows a progress indicator, not a + frozen UI.** Tapping Generate (or Regenerate/Regenerate all) shows a + spinner and disables the other generation actions until it finishes — + the app never appears to hang, even with a large synced catalogue + (previously this ran on the main thread; see Known regressions below). ### Known regressions @@ -239,6 +244,14 @@ None yet. `generateTeam` and `regenerateSlot` now treat `customSlots` as a reserved category, filled from the custom roster already loaded into `SurpriseMeViewModel`'s UI state. +- **Fixed 2026-09-05.** `AnalysisViewModel`'s coverage/suggestions + pipeline and both of `SurpriseMeViewModel`'s generator entry points ran + on `Dispatchers.Main.immediate` — found by code review + (`docs/post-migration-review.md`, finding 2), not by manual testing (no + device profiling backs the "real work" claim; it follows from the + algorithmic cost against a production-sized catalogue). Both now run on + `Dispatchers.Default`; Surprise Me additionally gained a progress + indicator and disables its generation actions while one is in flight. ## Phase 5 — Showdown import/export, settings and local backup From 2b6c6bf27bf08fc20a838ab8949cb12cb32770f3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 07:27:04 +0000 Subject: [PATCH 5/8] Hoist per-team work out of the suggestion/generator scoring loop (finding 3) computeCompositeScore took otherMembers: List and rebuilt the offensive coverage union and the weakness map from it on every call, even though neither depends on the candidate being scored - only on the team. In SuggestionEngine's replacement mode (a full team of six), every one of the N candidates recomputed all 6 possible "team minus one member" contexts from scratch, when only 6 distinct values are possible: N x 6 recomputations of the same 6 things. Extracted that half into a TeamScoringContext, built once per distinct team via teamScoringContext(chart, otherMembers) and passed into computeCompositeScore in place of the raw member list: - SuggestionEngine's addition mode builds one context before scoring every candidate (previously recomputed per candidate too). - SuggestionEngine's replacement mode builds the 6 "team minus one member" contexts once before the candidate loop, not once per candidate. - TeamGenerator.computeScore takes a context instead of the team list, built once per slot (generateTeam) or once per regenerateSlot call, not once per candidate scored in that iteration. It also reuses the context's baseCoverage as its own currentTeamCoverage argument, removing a second independent computation of the exact same set that existed only in that call site - documented in implementation-decisions.md why that reuse is specific to the generator and would be wrong in the suggestion engine, where currentTeamCoverage is analyseTeam's potentially moves-aware unionCovered, not the context's always-types-only baseCoverage. Behaviour-preserving: same suggestions, same composite scores, same ranking - every call site funnels through computeSuggestions/ generateTeam/regenerateSlot, all three already covered by SuggestionEngineTest/TeamGeneratorTest's exact-score and exact-ranking assertions, so no new test was added for this commit. Full analysis: docs/post-migration-review.md, finding 3. Same verification caveat as the prior commits in this series: no Android SDK in this sandbox, so this refactor is unverified by a local run; CI is watched. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_018zXVpxEbSWadqKyntXTXD6 --- CHANGELOG.md | 7 +++ .../domain/generator/TeamGenerator.kt | 39 +++++++------- .../coverdex/domain/suggestion/Scoring.kt | 53 ++++++++++++------- .../domain/suggestion/SuggestionEngine.kt | 24 +++++---- docs/implementation-decisions.md | 24 +++++++++ docs/test-plan.md | 9 ++++ 6 files changed, 109 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 895a6f2..3a1463b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,13 @@ versions follow [Semantic Versioning](https://semver.org/). [`docs/post-migration-review.md`](docs/post-migration-review.md). Surprise Me now shows a progress indicator and disables its Generate/Regenerate actions while a generation is in flight. +- **Suggestions in replacement mode (a full team of six) score + noticeably faster.** `computeCompositeScore` recomputed the same + per-team data from scratch for every candidate — in replacement mode, + six times per candidate instead of once overall. See finding 3 in + [`docs/post-migration-review.md`](docs/post-migration-review.md). Same + suggestions, same ranking, same composite scores — this is a pure + performance fix. - **Post-migration review of the coverage and suggestion engines.** A code-level audit of the shipped app against the phase plans, diffing every Kotlin port against the TypeScript original recovered from git history. diff --git a/app/src/main/java/com/marcogn/coverdex/domain/generator/TeamGenerator.kt b/app/src/main/java/com/marcogn/coverdex/domain/generator/TeamGenerator.kt index e01dfd9..57aa6fb 100644 --- a/app/src/main/java/com/marcogn/coverdex/domain/generator/TeamGenerator.kt +++ b/app/src/main/java/com/marcogn/coverdex/domain/generator/TeamGenerator.kt @@ -1,12 +1,12 @@ package com.marcogn.coverdex.domain.generator -import com.marcogn.coverdex.domain.coverage.offensiveCoverageForMember import com.marcogn.coverdex.domain.model.PokemonEntry -import com.marcogn.coverdex.domain.model.PokemonType import com.marcogn.coverdex.domain.model.TeamMember import com.marcogn.coverdex.domain.model.TypeChart +import com.marcogn.coverdex.domain.suggestion.TeamScoringContext import com.marcogn.coverdex.domain.suggestion.computeCompositeScore import com.marcogn.coverdex.domain.suggestion.memberFromEntry +import com.marcogn.coverdex.domain.suggestion.teamScoringContext import kotlin.random.Random /** @@ -77,20 +77,16 @@ private data class Candidate(val member: TeamMember, val entry: PokemonEntry?, v private fun candidateFromEntry(entry: PokemonEntry): Candidate = Candidate(memberFromEntry(entry), entry, entry.defaultAbility) private fun candidateFromCustom(member: TeamMember): Candidate = Candidate(member, null, member.ability) -private fun currentTeamCoverage(chart: TypeChart, team: List): Set { - val cov = mutableSetOf() - team.forEach { cov.addAll(offensiveCoverageForMember(chart, it, false)) } - return cov -} - -/** Composite score for [candidate] relative to [currentTeam], plus a small random tie-breaking - * factor. Ports `teamGenerator.ts`'s own `computeScore` as a thin wrapper over the shared - * [computeCompositeScore] — `currentTeam` doubles as both the "other members" and the coverage - * baseline, since the generator (unlike the suggestion engine's replacement mode) never excludes - * a member from the comparison. */ -private fun computeScore(chart: TypeChart, candidate: TeamMember, currentTeam: List, random: Random): Double { - val coverage = currentTeamCoverage(chart, currentTeam) - val result = computeCompositeScore(chart, candidate, currentTeam, coverage) +/** Composite score for [candidate] against a team summarized by [context] (see + * [teamScoringContext]), plus a small random tie-breaking factor. Ports `teamGenerator.ts`'s own + * `computeScore` as a thin wrapper over the shared [computeCompositeScore] — [context]'s + * `baseCoverage` doubles as both the "other members" coverage and the gain baseline, since the + * generator (unlike the suggestion engine's replacement mode) never excludes a member from the + * comparison, so building [context] from the exact team being scored against makes the two the + * same set. Callers build [context] once per team (not once per candidate) — see + * `docs/post-migration-review.md`, finding 3. */ +private fun computeScore(chart: TypeChart, candidate: TeamMember, context: TeamScoringContext, random: Random): Double { + val result = computeCompositeScore(chart, candidate, context, context.baseCoverage) val noise = (random.nextDouble() - 0.5) * 0.02 return result.compositeScore + noise } @@ -188,10 +184,13 @@ fun generateTeam( return GeneratorResult(team = team, warning = "tooFewPokemon") } + // Built once per slot, not once per candidate — see docs/post-migration-review.md, + // finding 3. + val context = teamScoringContext(chart, team) // maxByOrNull calls its selector exactly once per element (never per comparison), so this // is safe even though computeScore adds fresh random noise per call — see regenerateSlot's // own note below on the sort that must not do the same thing the same way. - val best = candidates.maxByOrNull { candidate -> computeScore(chart, candidate.member, team, random) }!! + val best = candidates.maxByOrNull { candidate -> computeScore(chart, candidate.member, context, random) }!! team.add(best.member.copy(ability = best.ability)) usedSpecies.add(best.member.speciesName.lowercase()) @@ -262,13 +261,17 @@ fun regenerateSlot( return currentTeam[slotIndex] } + // Built once for every candidate below, not once per candidate — see + // docs/post-migration-review.md, finding 3. + val context = teamScoringContext(chart, otherMembers) + // computeScore adds fresh random noise per call, so it must be evaluated exactly once per // candidate here and sorted on the stored value — sortedByDescending { computeScore(...) } // re-invokes the selector on every comparison, which breaks Comparator's contract and makes // Collections.sort's TimSort throw once the pool is large enough to leave insertion-sort // territory (candidatePool is the full eligible catalogue here, not a test-sized fixture). val scored = candidatePool - .map { candidate -> candidate to computeScore(chart, candidate.member, otherMembers, random) } + .map { candidate -> candidate to computeScore(chart, candidate.member, context, random) } .sortedByDescending { (_, score) -> score } val topN = minOf(5, scored.size) diff --git a/app/src/main/java/com/marcogn/coverdex/domain/suggestion/Scoring.kt b/app/src/main/java/com/marcogn/coverdex/domain/suggestion/Scoring.kt index c7e6aa6..604c7bb 100644 --- a/app/src/main/java/com/marcogn/coverdex/domain/suggestion/Scoring.kt +++ b/app/src/main/java/com/marcogn/coverdex/domain/suggestion/Scoring.kt @@ -33,26 +33,50 @@ data class CompositeScoreResult( ) /** - * Composite score for [candidate] joining a team alongside [otherMembers], measured against - * [currentTeamCoverage] (the real team's union coverage — kept as its own parameter rather than - * re-derived from [otherMembers], since replacement mode compares against the *actual* team, not - * the hypothetical one with a member removed). Ports `suggestionEngine.ts`'s - * `computeCompositeScore` verbatim, [candidate] and [otherMembers] evaluated by types only - * (`offensiveCoverageForMember(chart, member, useMoves = false)`), never by moves — see + * The half of [computeCompositeScore]'s work that depends only on a team, never on the candidate + * being scored against it — build once per distinct `otherMembers` set with [teamScoringContext] + * and reuse it across every candidate, instead of recomputing it from scratch for each one. Before + * this existed, the suggestion engine's replacement mode (`SuggestionEngine.kt`) recomputed it + * N × 6 times (once per candidate, per team member considered for replacement) when only 6 distinct + * values are possible; see `docs/post-migration-review.md`, finding 3. + */ +class TeamScoringContext internal constructor( + val baseCoverage: Set, + val otherWeaknessMap: Map>, +) + +fun teamScoringContext(chart: TypeChart, otherMembers: List): TeamScoringContext { + val baseCov = mutableSetOf() + otherMembers.forEach { baseCov.addAll(offensiveCoverageForMember(chart, it, false)) } + + val otherWeaknessMap = mutableMapOf>() + for (m in otherMembers) { + for (w in weaknesses(chart, m.types)) { + otherWeaknessMap.getOrPut(w) { mutableListOf() }.add(m.speciesName) + } + } + return TeamScoringContext(baseCov, otherWeaknessMap) +} + +/** + * Composite score for [candidate] joining a team whose other members are summarized by [context] + * (see [teamScoringContext]), measured against [currentTeamCoverage] (the real team's union + * coverage — kept as its own parameter rather than re-derived from the context, since replacement + * mode compares against the *actual* team, not the hypothetical one with a member removed). Ports + * `suggestionEngine.ts`'s `computeCompositeScore` verbatim, [candidate] and the team evaluated by + * types only (`offensiveCoverageForMember(chart, member, useMoves = false)`), never by moves — see * `phase-4-suggestions-and-generator.md` §1.7. */ fun computeCompositeScore( chart: TypeChart, candidate: TeamMember, - otherMembers: List, + context: TeamScoringContext, currentTeamCoverage: Set, ): CompositeScoreResult { val candCov = offensiveCoverageForMember(chart, candidate, false) - val baseCov = mutableSetOf() - otherMembers.forEach { baseCov.addAll(offensiveCoverageForMember(chart, it, false)) } val newUnion = mutableSetOf().apply { - addAll(baseCov) + addAll(context.baseCoverage) addAll(candCov) } @@ -61,19 +85,12 @@ fun computeCompositeScore( val candWeaknesses = weaknesses(chart, candidate.types) - val otherWeaknessMap = mutableMapOf>() - for (m in otherMembers) { - for (w in weaknesses(chart, m.types)) { - otherWeaknessMap.getOrPut(w) { mutableListOf() }.add(m.speciesName) - } - } - val newWeaknesses = mutableListOf() val aggravatedWeaknesses = mutableListOf() val aggravatedMembers = mutableMapOf>() for (w in candWeaknesses) { - val membersWithSameWeakness = otherWeaknessMap[w] + val membersWithSameWeakness = context.otherWeaknessMap[w] if (!membersWithSameWeakness.isNullOrEmpty()) { aggravatedWeaknesses.add(w) aggravatedMembers[w] = membersWithSameWeakness diff --git a/app/src/main/java/com/marcogn/coverdex/domain/suggestion/SuggestionEngine.kt b/app/src/main/java/com/marcogn/coverdex/domain/suggestion/SuggestionEngine.kt index 83a09e3..cc315d1 100644 --- a/app/src/main/java/com/marcogn/coverdex/domain/suggestion/SuggestionEngine.kt +++ b/app/src/main/java/com/marcogn/coverdex/domain/suggestion/SuggestionEngine.kt @@ -120,8 +120,11 @@ fun computeSuggestions( val deduped = dedupCandidates.filter { seen.add(it.speciesName.lowercase()) } val ranked: List = if (members.size < 6) { + // Every candidate is scored against the same team, so this is built once, not once per + // candidate — see docs/post-migration-review.md, finding 3. + val context = teamScoringContext(chart, members) deduped.map { cand -> - val result = computeCompositeScore(chart, cand, members, teamAnalysis.unionCovered) + val result = computeCompositeScore(chart, cand, context, teamAnalysis.unionCovered) val entry = findEntry(pool, cand.speciesName) RankedCandidate( candidate = cand, @@ -133,18 +136,17 @@ fun computeSuggestions( ) } } else { + // Only 6 distinct "team minus one member" contexts exist, regardless of how many + // candidates are scored against them — build each one once, not once per candidate. Before + // this, every one of the N candidates recomputed all 6 from scratch (N × 6 calls instead + // of 6); see docs/post-migration-review.md, finding 3. + val replacementContexts = members.map { m -> m to teamScoringContext(chart, members.filter { it.id != m.id }) } deduped.map { cand -> var bestScore = Double.NEGATIVE_INFINITY - var bestMember = members[0] - var bestResult = computeCompositeScore( - chart, - cand, - members.filter { it.id != members[0].id }, - teamAnalysis.unionCovered, - ) - for (m in members) { - val otherMembers = members.filter { it.id != m.id } - val result = computeCompositeScore(chart, cand, otherMembers, teamAnalysis.unionCovered) + var bestMember = replacementContexts[0].first + var bestResult = computeCompositeScore(chart, cand, replacementContexts[0].second, teamAnalysis.unionCovered) + for ((m, context) in replacementContexts) { + val result = computeCompositeScore(chart, cand, context, teamAnalysis.unionCovered) if (result.compositeScore > bestScore) { bestScore = result.compositeScore bestMember = m diff --git a/docs/implementation-decisions.md b/docs/implementation-decisions.md index b8012ea..aedca6e 100644 --- a/docs/implementation-decisions.md +++ b/docs/implementation-decisions.md @@ -762,6 +762,30 @@ findings not yet acted on. observed via a real `StateFlow` update from a real background thread — not virtual time — the same category of wait every Room-/DataStore- backed test in this codebase already relies on. +- **Finding 3 (per-candidate rework in `computeCompositeScore`) — + `TeamScoringContext` extracted, no new test.** `computeCompositeScore` + took `otherMembers: List` and rebuilt the offensive + coverage union and weakness map from it on every call, even though + neither depends on the candidate — only on the team. Extracted that + half into `teamScoringContext(chart, otherMembers): TeamScoringContext`, + built once per distinct team (once in addition mode, once per + replacement candidate — six total, not `candidates × 6`) and passed + into `computeCompositeScore` in place of the raw member list. + `TeamGenerator.computeScore` additionally reused the context's + `baseCoverage` as its own `currentTeamCoverage` argument, deleting a + second, independent redundant computation of the exact same set that + existed only in that one call site (the generator never excludes a + member from the comparison the way the suggestion engine's replacement + mode does, so building the context from the exact team scored against + makes the two values identical — this does **not** hold for the + suggestion engine, where `currentTeamCoverage` is `analyseTeam`'s + potentially moves-aware `unionCovered`, deliberately different from + the context's always-types-only `baseCoverage`). No new test: this is + behaviour-preserving by construction (same inputs, same output shape, + work reordered not changed), and every call site funnels through + `computeSuggestions`/`generateTeam`/`regenerateSlot`, all three already + covered by `SuggestionEngineTest`/`TeamGeneratorTest`'s existing exact- + score and exact-ranking assertions. - **Finding 6 (abilities ignored by suggestion/generator scoring) — not yet acted on.** Left for its own follow-up commit since it changes the composite score's output, not just its performance or a missing diff --git a/docs/test-plan.md b/docs/test-plan.md index 1b9c973..ee93c4b 100644 --- a/docs/test-plan.md +++ b/docs/test-plan.md @@ -252,6 +252,15 @@ None yet. algorithmic cost against a production-sized catalogue). Both now run on `Dispatchers.Default`; Surprise Me additionally gained a progress indicator and disables its generation actions while one is in flight. +- **Fixed 2026-09-05.** `computeCompositeScore` redid the same per-team + work (offensive coverage union, weakness map) from scratch for every + candidate — found by code review + (`docs/post-migration-review.md`, finding 3), not measured on a + device. In replacement mode (a full team of six) that meant six + redundant recomputations per candidate instead of one overall. + Behaviour-preserving: same suggestions, same scores, same ranking — + covered by the existing `SuggestionEngineTest`/`TeamGeneratorTest` + suites, no new test needed. ## Phase 5 — Showdown import/export, settings and local backup From 4dc2e915e4c302ec9693228e3e6c71654b37b133 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 07:30:41 +0000 Subject: [PATCH 6/8] Correct finding 4 and add the solid-coverage message The post-migration review's original finding 4 claimed the Suggestions panel was missing the PWA's type-filter chips, its "Best coverage"/ "Random" mode toggle, and showed 5 cards where the PWA showed 10 - based on diffing against SuggestionPanel.android.tsx/ SuggestionFilters.android.tsx alone, without checking this rewrite's own spec first. docs/plan/native-spec.md's "Suggestion engine" section says, verbatim, "Return the top 5 by gain" for both addition and replacement mode, and specifies neither the type filter nor the mode toggle. SuggestionFilters.kt's own doc comment already said as much ("Deliberately smaller than legacy-web's own SuggestionFilters.tsx [...]: neither is in this app's UI spec") - a comment that should have been read before writing that finding, and wasn't. Five cards is the spec for this rewrite, not a shortfall against the PWA. Implementing the retracted items would have reversed a documented Phase 4 decision instead of fixing a migration defect. Corrected the finding in docs/post-migration-review.md (kept, not deleted, with the correction stated plainly) and recorded the same in implementation-decisions.md and the test-plan's Known regressions, since this was already reported to the user as a defect before the correction. What survives, both narrow and independent of the retracted items: - Added the "solid coverage" message (shown when every displayed suggestion has zero gain) to AnalysisScreen.kt, with new suggestions_solid_coverage strings in both locales. - Removed suggestions_exclude_legendaries, a genuinely orphaned string resource referenced by no composable (the real toggle lives in Settings under the inverted settings_include_legendaries framing). No domain logic changed; this is UI + string resources only. No new test: no Compose screen tests exist in this codebase for this class of change (per CLAUDE.md, screen-level behaviour is verified by hand, see test-plan.md), consistent with how AnalysisScreen's other conditional messages (e.g. suggestions_no_suggestions) are already covered. Same verification caveat as the other commits in this series: no Android SDK in this sandbox, so this change is unverified by a local run; CI is watched. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_018zXVpxEbSWadqKyntXTXD6 --- CHANGELOG.md | 10 +++ .../ui/team/analysis/AnalysisScreen.kt | 10 ++- app/src/main/res/values-en/strings.xml | 2 +- app/src/main/res/values/strings.xml | 2 +- docs/implementation-decisions.md | 22 +++++ docs/post-migration-review.md | 80 +++++++++++-------- docs/test-plan.md | 16 ++++ 7 files changed, 107 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a1463b..84e6db3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,16 @@ versions follow [Semantic Versioning](https://semver.org/). [`docs/post-migration-review.md`](docs/post-migration-review.md). Same suggestions, same ranking, same composite scores — this is a pure performance fix. +- **Suggestions show a "coverage is already solid" note when every + displayed card offers zero gain**, instead of five zero-gain cards + with no framing. (An earlier draft of this entry also claimed the + Suggestions panel was missing a type filter, a random mode, and showed + 5 cards where it should show 10 — `docs/plan/native-spec.md` says "top + 5" for this rewrite and specifies neither of the other two features, + so those were not implemented; see finding 4's correction in + [`docs/post-migration-review.md`](docs/post-migration-review.md).) + Also removed one orphaned string resource + (`suggestions_exclude_legendaries`) left over from Phase 4. - **Post-migration review of the coverage and suggestion engines.** A code-level audit of the shipped app against the phase plans, diffing every Kotlin port against the TypeScript original recovered from git history. diff --git a/app/src/main/java/com/marcogn/coverdex/ui/team/analysis/AnalysisScreen.kt b/app/src/main/java/com/marcogn/coverdex/ui/team/analysis/AnalysisScreen.kt index f6836db..c70d7c5 100644 --- a/app/src/main/java/com/marcogn/coverdex/ui/team/analysis/AnalysisScreen.kt +++ b/app/src/main/java/com/marcogn/coverdex/ui/team/analysis/AnalysisScreen.kt @@ -151,8 +151,16 @@ fun AnalysisScreen(modifier: Modifier = Modifier, viewModel: AnalysisViewModel = color = MaterialTheme.colorScheme.onSurfaceVariant, ) } else { + val displayed = state.suggestions.take(5) Column(verticalArrangement = Arrangement.spacedBy(8.dp)) { - state.suggestions.take(5).forEach { suggestion -> + if (displayed.all { it.gain == 0 }) { + Text( + stringResource(R.string.suggestions_solid_coverage), + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + ) + } + displayed.forEach { suggestion -> SuggestionCard(suggestion = suggestion, onApply = viewModel::applySuggestion) } } diff --git a/app/src/main/res/values-en/strings.xml b/app/src/main/res/values-en/strings.xml index 65f5d73..315eb2c 100644 --- a/app/src/main/res/values-en/strings.xml +++ b/app/src/main/res/values-en/strings.xml @@ -87,7 +87,7 @@ All generations Generation %1$d Include custom Pokémon - Exclude legendaries/mythicals + Your team coverage is already solid. Alternatives: Add to team Replaces: %1$s Covers: diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 7eeb032..94b7e01 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -87,7 +87,7 @@ Tutte le generazioni Generazione %1$d Includi Pokémon custom - Escludi leggendari/mitici + La copertura del team è già solida. Alternative: Aggiungi al team Sostituisci: %1$s Copre: diff --git a/docs/implementation-decisions.md b/docs/implementation-decisions.md index aedca6e..c13f1df 100644 --- a/docs/implementation-decisions.md +++ b/docs/implementation-decisions.md @@ -786,6 +786,28 @@ findings not yet acted on. `computeSuggestions`/`generateTeam`/`regenerateSlot`, all three already covered by `SuggestionEngineTest`/`TeamGeneratorTest`'s existing exact- score and exact-ranking assertions. +- **Finding 4 (Suggestions panel "missing" PWA features) — the original + finding was wrong, corrected before implementing it.** As first + written, finding 4 claimed the Suggestions panel was missing a + type-filter chip row, a "Best coverage"/"Random" mode toggle, and + showed 5 cards where the PWA showed 10 — based on a diff against + `SuggestionPanel.android.tsx`/`SuggestionFilters.android.tsx` alone. + `docs/plan/native-spec.md`'s "Suggestion engine" section states "Return + the top 5 by `gain`" for both addition and replacement mode, and + mentions neither the type filter nor the mode toggle; + `SuggestionFilters.kt`'s own doc comment already said as much + ("Deliberately smaller than `legacy-web`'s own `SuggestionFilters.tsx` + [...]: neither is in this app's UI spec") — a comment the original + review should have read before writing that table and didn't. Five + cards is the spec for this rewrite, not a shortfall against the PWA; + implementing the retracted items would have reversed a documented + Phase 4 decision. What survived the correction: a `solidCoverage` + message (independent of the retracted items — shown when every + displayed suggestion has zero gain, regardless of how many are shown or + how they got filtered) and deleting one genuinely orphaned string + resource (`suggestions_exclude_legendaries`). See the corrected finding + 4 in `docs/post-migration-review.md` for the full record, including the + quoted spec text. - **Finding 6 (abilities ignored by suggestion/generator scoring) — not yet acted on.** Left for its own follow-up commit since it changes the composite score's output, not just its performance or a missing diff --git a/docs/post-migration-review.md b/docs/post-migration-review.md index 9da8967..0f9ea54 100644 --- a/docs/post-migration-review.md +++ b/docs/post-migration-review.md @@ -158,36 +158,51 @@ Roughly a 5× reduction, and it composes with finding 2 rather than replacing it. Behaviour-preserving: the existing `SuggestionEngineTest`/`TeamGeneratorTest` suites are the regression net. -### 4. The Suggestions panel is missing features the PWA shipped - -Diffing `SuggestionPanel.android.tsx` / `SuggestionFilters.android.tsx` -against `ui/team/analysis/SuggestionFilters.kt` and `AnalysisScreen.kt:155`: - -| PWA behaviour | Native | -|---|---| -| Type filter chips (`filterByType`) — narrow suggestions to chosen types | **missing** | -| "Best coverage" / "Random" mode toggle + "Randomize again" (`pickRandom(filtered, 10)`) | **missing** | -| `suggestions.noMatch` — "No suggestions match current filters." | **missing** | -| `suggestions.solidCoverage` — shown when every displayed card has `gain == 0` | **missing** | -| Shows `filtered.slice(0, 10)` | shows `take(5)` | - -The corresponding keys are absent from both `res/values/strings.xml` and -`res/values-en/strings.xml` (`bestCoverage`, `random`, `randomizeAgain`, -`noMatch`, `solidCoverage`, `newTypesCovered`). The mixed-moves note *was* -ported (`analysis_basis_mixed`). - -`computeSuggestions` already returns every ranked candidate uncapped, so -the type filter and the random mode are pure UI work over data that is -already there. - -Also orphaned: `suggestions_exclude_legendaries` is defined in both locale -files and referenced by no composable — the toggle actually lives in -Settings under the inverted `settings_include_legendaries` framing. - -**Fix.** Restore the two filters and the two contextual messages, raise the -cut to 10, and either wire up or delete the orphan string. Whether the -"random" mode is still wanted is a product call; if not, say so in -`implementation-decisions.md` rather than leaving it undocumented. +### 4. The Suggestions panel — corrected after further review + +**This finding was substantially wrong as first written, and the record +needs to say so plainly rather than quietly fixing it.** The original +version of this section, based on a diff against +`SuggestionPanel.android.tsx`/`SuggestionFilters.android.tsx`, claimed the +native Suggestions panel was "missing" the PWA's type-filter chips, +"Best coverage"/"Random" mode toggle, and a 10-card cut (native shows 5). +Before implementing any of that, `docs/plan/native-spec.md`'s own +"Suggestion engine" section turned up and settles it: + +> **Team size < 6 — addition mode.** [...] Return the top 5 by `gain`. +> **Team size = 6 — replacement mode.** [...] Return the top 5 by `gain`, +> keyed by species name so a species cannot appear twice. + +Five cards is the spec, not a shortfall against it — the PWA's `slice(0, +10)` is exactly the kind of legacy-web behavior this rewrite was never +bound to reproduce. Neither the type filter nor the best/random toggle +appears anywhere in `native-spec.md` either, and +`ui/team/analysis/SuggestionFilters.kt` already carries a doc comment +saying as much: "Deliberately smaller than `legacy-web`'s own +`SuggestionFilters.tsx` [...]: neither is in this app's UI spec." That +comment was sitting in the file this review diffed against and should +have been read before writing the original table above — it wasn't. So: +**no type filter, no random mode, no 10-card cut** — implementing any of +them would be reversing a documented Phase 4 decision, not restoring a +migration defect. + +What survives the correction, both narrow and independent of the retracted +items: + +- **`suggestions.solidCoverage`** ("Your team coverage is solid. Showing + alternatives:") — shown by the PWA when every displayed card has + `gain == 0`. This doesn't depend on the type filter or the cut count; + it's a small, orthogonal contextual message a team with complete + offensive coverage would otherwise lack, seeing only a stack of + zero-gain cards with no framing. Legitimate, optional, low-risk to add. +- **`suggestions_exclude_legendaries`** is a genuinely orphaned string + resource — defined in both locale files, referenced by no composable + (the real toggle lives in Settings under the inverted + `settings_include_legendaries` framing). Unrelated to the rest of this + finding; worth deleting on its own merits. + +**Fix.** Add the solid-coverage message; delete the orphan string. Nothing +else from the original table. ### 5. The "Custom slots" stepper does nothing @@ -258,8 +273,9 @@ than load-bearing. **Then — parity and accuracy** -5. Finding 4: restore the type filter, the two contextual messages and the - 10-card cut; decide on the random mode; clear the orphan string. +5. Finding 4 (corrected): add the solid-coverage message; clear the orphan + string. The type filter, random mode and 10-card cut are **not** part + of this — see the correction in finding 4 itself. 6. Finding 6: thread abilities through the scoring path, as a documented spec change. diff --git a/docs/test-plan.md b/docs/test-plan.md index ee93c4b..a3f3cae 100644 --- a/docs/test-plan.md +++ b/docs/test-plan.md @@ -200,6 +200,10 @@ None yet. - [ ] **No suggestions message.** A team with nothing left to suggest (or an empty synced catalogue) shows the "no suggestions" message instead of an empty section. +- [ ] **Solid coverage message.** A team whose offensive coverage is + already complete shows "Your team coverage is already solid. + Alternatives:" above the five suggestion cards (all showing zero + gain), rather than five zero-gain cards with no framing. - [ ] **Surprise Me — anchors.** From Teams, tap the dice icon. Search and lock up to 5 Pokémon; each appears as a removable chip. Locking a 6th is blocked with the "all slots locked" warning shown. @@ -261,6 +265,18 @@ None yet. Behaviour-preserving: same suggestions, same scores, same ranking — covered by the existing `SuggestionEngineTest`/`TeamGeneratorTest` suites, no new test needed. +- **Corrected 2026-09-05, not a regression.** The post-migration review's + original finding 4 claimed the Suggestions panel was missing a + type-filter, a best/random mode toggle, and showed 5 cards where the + PWA showed 10. `docs/plan/native-spec.md`'s own "Suggestion engine" + section says "Return the top 5 by `gain`" for both addition and + replacement mode, and none of the other three items appear in it + either — the current behaviour is spec-compliant, not a shortfall. See + the corrected finding 4 in `docs/post-migration-review.md`. Added only + the one narrow, independent item that survived the correction: a + "solid coverage" message when every displayed suggestion has zero + gain; also removed one genuinely orphaned string resource + (`suggestions_exclude_legendaries`, referenced by no composable). ## Phase 5 — Showdown import/export, settings and local backup From 9be5dbb4c6c1db88a4f9ba548ddfa334705ef9b0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 07:36:28 +0000 Subject: [PATCH 7/8] Honour scoring-relevant abilities in weaknesses() (finding 6) weaknesses(chart, types) never took an ability, unlike CoverageEngine's own sharedWeaknessCounts/defensiveProfile, which always did - so one Analysis screen could disagree with itself: the coverage grid would show a member immune to a type via its ability, while the Suggestions section, fed by the same team, still penalized a candidate for "aggravating" that exact weakness. - Scoring.kt: weaknesses() takes an optional ability parameter. computeCompositeScore's candWeaknesses now passes candidate.ability; teamScoringContext's otherWeaknessMap now passes each member's own .ability when building it. - SuggestionEngine.kt: memberFromEntry now sets ability = e.defaultAbility instead of always null, so a suggested/generated candidate is scored with the ability it will actually carry once applied, not with none. This is a native addition on top of the port, not a behavior the TypeScript ever had. - TeamGenerator.kt: Candidate drops its own separate ability field - both candidateFromEntry and candidateFromCustom now produce a member whose own .ability is already correct, so the field that existed only to override it on pick is dead weight. team.add(best.member.copy(...)) and the regenerateSlot equivalent simplify to just the member itself. This is a real spec change, not a refactor: composite scores now differ from the ported TypeScript baseline for any team member or candidate carrying one of AbilityEffects.kt's 14 known scoring-relevant abilities. Every existing fixture across TestFixtures.kt, SuggestionEngineTest.kt and TeamGeneratorTest.kt has defaultAbility = null and builds every TeamMember with no explicit ability either, so this change is invisible to every existing exact-score/exact-ranking assertion - confirmed by grep before writing this commit, not assumed. Added ScoringTest.kt to exercise the new behavior directly: Levitate removing a candidate's own Ground weakness, and a teammate's Levitate changing a shared candidate weakness from "aggravated" (1.0 penalty) to merely "new" (0.5). Documented as a deliberate spec change in implementation-decisions.md and docs/test-plan.md, per the review's own instruction not to bundle it with an unrelated fix. Full analysis: docs/post-migration-review.md, finding 6 (also updated in this commit: the Plan section now marks all six findings done, and the Verdict section's stray "five findings" is corrected to six). Same verification caveat as the other commits in this series: no Android SDK in this sandbox, so this change and its new test are unverified by a local run; CI is watched. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_018zXVpxEbSWadqKyntXTXD6 --- CHANGELOG.md | 9 ++ .../domain/generator/TeamGenerator.kt | 23 ++--- .../coverdex/domain/suggestion/Scoring.kt | 18 ++-- .../domain/suggestion/SuggestionEngine.kt | 8 +- .../coverdex/domain/suggestion/ScoringTest.kt | 85 +++++++++++++++++++ docs/implementation-decisions.md | 34 ++++++-- docs/post-migration-review.md | 68 +++++++++------ docs/test-plan.md | 14 +++ 8 files changed, 212 insertions(+), 47 deletions(-) create mode 100644 app/src/test/java/com/marcogn/coverdex/domain/suggestion/ScoringTest.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index 84e6db3..bcfe2b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,15 @@ versions follow [Semantic Versioning](https://semver.org/). [`docs/post-migration-review.md`](docs/post-migration-review.md).) Also removed one orphaned string resource (`suggestions_exclude_legendaries`) left over from Phase 4. +- **Suggestions and Surprise Me now honour a scoring-relevant ability + the same way the Analysis screen's coverage grid already did.** A + Levitate-holding teammate's Ground weakness no longer counts as + "aggravated" against a candidate that shares it, and a candidate whose + own ability removes a weakness is no longer penalized for it — one + screen could previously disagree with itself on this. This changes + composite scores for any team or candidate with an ability from + `AbilityEffects.kt`'s known-effects list; see finding 6 in + [`docs/post-migration-review.md`](docs/post-migration-review.md). - **Post-migration review of the coverage and suggestion engines.** A code-level audit of the shipped app against the phase plans, diffing every Kotlin port against the TypeScript original recovered from git history. diff --git a/app/src/main/java/com/marcogn/coverdex/domain/generator/TeamGenerator.kt b/app/src/main/java/com/marcogn/coverdex/domain/generator/TeamGenerator.kt index 57aa6fb..cf2e332 100644 --- a/app/src/main/java/com/marcogn/coverdex/domain/generator/TeamGenerator.kt +++ b/app/src/main/java/com/marcogn/coverdex/domain/generator/TeamGenerator.kt @@ -68,14 +68,17 @@ private fun isMegaOrDynamax(entry: PokemonEntry): Boolean = isMega(entry) || isD private fun findEntry(allPokemon: List, m: TeamMember): PokemonEntry? = allPokemon.find { it.displayName == m.speciesName || it.name == m.speciesName.lowercase() } -/** A generator candidate: either a catalogue entry (scored via [memberFromEntry], its ability - * taken from [PokemonEntry.defaultAbility]) or a custom roster [TeamMember] (its own ability kept - * as-is). [entry] is `null` for a custom — that is the single source of truth this file uses to - * tell the two apart, since a custom is never legendary/mythical, a starter, a Mega or a Dynamax. */ -private data class Candidate(val member: TeamMember, val entry: PokemonEntry?, val ability: String?) - -private fun candidateFromEntry(entry: PokemonEntry): Candidate = Candidate(memberFromEntry(entry), entry, entry.defaultAbility) -private fun candidateFromCustom(member: TeamMember): Candidate = Candidate(member, null, member.ability) +/** A generator candidate: either a catalogue entry (scored via [memberFromEntry], which already + * carries [PokemonEntry.defaultAbility] as its own [TeamMember.ability]) or a custom roster + * [TeamMember] (its own ability, unchanged). [entry] is `null` for a custom — that is the single + * source of truth this file uses to tell the two apart, since a custom is never legendary/ + * mythical, a starter, a Mega or a Dynamax. No separate ability field: [member]'s own already + * carries the right value in both cases, so there is nothing left to override when a candidate is + * picked. */ +private data class Candidate(val member: TeamMember, val entry: PokemonEntry?) + +private fun candidateFromEntry(entry: PokemonEntry): Candidate = Candidate(memberFromEntry(entry), entry) +private fun candidateFromCustom(member: TeamMember): Candidate = Candidate(member, null) /** Composite score for [candidate] against a team summarized by [context] (see * [teamScoringContext]), plus a small random tie-breaking factor. Ports `teamGenerator.ts`'s own @@ -192,7 +195,7 @@ fun generateTeam( // own note below on the sort that must not do the same thing the same way. val best = candidates.maxByOrNull { candidate -> computeScore(chart, candidate.member, context, random) }!! - team.add(best.member.copy(ability = best.ability)) + team.add(best.member) usedSpecies.add(best.member.speciesName.lowercase()) if (best.entry != null) { @@ -277,5 +280,5 @@ fun regenerateSlot( val topN = minOf(5, scored.size) val picked = scored[random.nextInt(topN)].first - return picked.member.copy(ability = picked.ability) + return picked.member } diff --git a/app/src/main/java/com/marcogn/coverdex/domain/suggestion/Scoring.kt b/app/src/main/java/com/marcogn/coverdex/domain/suggestion/Scoring.kt index 604c7bb..650a2c0 100644 --- a/app/src/main/java/com/marcogn/coverdex/domain/suggestion/Scoring.kt +++ b/app/src/main/java/com/marcogn/coverdex/domain/suggestion/Scoring.kt @@ -18,10 +18,16 @@ import com.marcogn.coverdex.domain.model.TypeChart const val NEW_WEAKNESS_PENALTY = 0.5 const val AGGRAVATED_WEAKNESS_PENALTY = 1.0 -/** Types weak (>1x) against [types], evaluated by types only — the same defensive check - * `suggestionEngine.ts` and `teamGenerator.ts` both duplicate as a private `getWeaknesses`. */ -fun weaknesses(chart: TypeChart, types: Pair): List = - PokemonType.entries.filter { atk -> defensiveMultiplier(chart, atk, types) > 1.0 } +/** Types weak (>1x) against [types] with [ability] applied — the same defensive check + * `suggestionEngine.ts` and `teamGenerator.ts` both duplicate as a private `getWeaknesses`, except + * both TypeScript originals (and this function, before this ability parameter was added) ignored + * ability entirely, unlike `sharedWeaknessCounts`/`defensiveProfile` in `CoverageEngine.kt`, which + * always honoured it — see `docs/post-migration-review.md`, finding 6, and + * `docs/implementation-decisions.md`, "Post-migration review", for why this is a deliberate spec + * change (composite scores now differ from the ported baseline for any member/candidate with a + * scoring-relevant ability) rather than a pure refactor. */ +fun weaknesses(chart: TypeChart, types: Pair, ability: String? = null): List = + PokemonType.entries.filter { atk -> defensiveMultiplier(chart, atk, types, ability) > 1.0 } data class CompositeScoreResult( val compositeScore: Double, @@ -51,7 +57,7 @@ fun teamScoringContext(chart: TypeChart, otherMembers: List): TeamSc val otherWeaknessMap = mutableMapOf>() for (m in otherMembers) { - for (w in weaknesses(chart, m.types)) { + for (w in weaknesses(chart, m.types, m.ability)) { otherWeaknessMap.getOrPut(w) { mutableListOf() }.add(m.speciesName) } } @@ -83,7 +89,7 @@ fun computeCompositeScore( val offensiveGain = newUnion.size - currentTeamCoverage.size val newlyCovered = newUnion.filter { it !in currentTeamCoverage } - val candWeaknesses = weaknesses(chart, candidate.types) + val candWeaknesses = weaknesses(chart, candidate.types, candidate.ability) val newWeaknesses = mutableListOf() val aggravatedWeaknesses = mutableListOf() diff --git a/app/src/main/java/com/marcogn/coverdex/domain/suggestion/SuggestionEngine.kt b/app/src/main/java/com/marcogn/coverdex/domain/suggestion/SuggestionEngine.kt index cc315d1..39ac85d 100644 --- a/app/src/main/java/com/marcogn/coverdex/domain/suggestion/SuggestionEngine.kt +++ b/app/src/main/java/com/marcogn/coverdex/domain/suggestion/SuggestionEngine.kt @@ -48,13 +48,17 @@ data class SuggestionOptions( /** Turns a catalogue entry into a candidate [TeamMember] with no moves — candidates are always * evaluated by types only. Ports `memberFromEntry`; `spriteUrl` is dropped, since this codebase * derives sprite URLs from [TeamMember.pokedexId] rather than storing them (see - * `domain/sprite/SpriteUrlResolver.kt`), unlike the TypeScript `TeamMember.spriteUrl` field. */ + * `domain/sprite/SpriteUrlResolver.kt`), unlike the TypeScript `TeamMember.spriteUrl` field. + * [TeamMember.ability] is [PokemonEntry.defaultAbility] rather than the TypeScript's always-absent + * ability field — a deliberate native addition (not a port), since a candidate scored without the + * ability it would actually carry once applied is exactly finding 6 in + * `docs/post-migration-review.md`. */ fun memberFromEntry(e: PokemonEntry): TeamMember = TeamMember( id = "cand-${e.id}", pokedexId = e.id, speciesName = e.displayName, types = e.types, - ability = null, + ability = e.defaultAbility, moves = List(4) { null }, isCustomSaved = false, ) diff --git a/app/src/test/java/com/marcogn/coverdex/domain/suggestion/ScoringTest.kt b/app/src/test/java/com/marcogn/coverdex/domain/suggestion/ScoringTest.kt new file mode 100644 index 0000000..2ed94fd --- /dev/null +++ b/app/src/test/java/com/marcogn/coverdex/domain/suggestion/ScoringTest.kt @@ -0,0 +1,85 @@ +package com.marcogn.coverdex.domain.suggestion + +import com.marcogn.coverdex.domain.buildMember +import com.marcogn.coverdex.domain.mockTypeChart +import com.marcogn.coverdex.domain.model.PokemonType +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * `weaknesses` gained an [ability] parameter as a deliberate spec change — see + * `docs/post-migration-review.md`, finding 6, and `docs/implementation-decisions.md`, "Post- + * migration review". Before this, `computeCompositeScore` (and the suggestion/generator scoring + * built on it) could report a candidate as "aggravating" a teammate's weakness the coverage grid + * on the very same screen already excluded via that teammate's ability (e.g. Levitate's Ground + * immunity), or penalize a candidate for a weakness its own ability removes. + */ +class ScoringTest { + + private val chart = mockTypeChart() + + @Test + fun `weaknesses without an ability includes Ground for an Electric type`() { + assertTrue(weaknesses(chart, PokemonType.ELECTRIC to null).contains(PokemonType.GROUND)) + } + + @Test + fun `weaknesses with levitate excludes the Ground immunity`() { + val withoutAbility = weaknesses(chart, PokemonType.ELECTRIC to null) + val withLevitate = weaknesses(chart, PokemonType.ELECTRIC to null, "levitate") + assertTrue(withoutAbility.contains(PokemonType.GROUND)) + assertFalse(withLevitate.contains(PokemonType.GROUND)) + } + + @Test + fun `weaknesses with an unknown ability behaves exactly like no ability`() { + val withoutAbility = weaknesses(chart, PokemonType.ELECTRIC to null) + val withUnknownAbility = weaknesses(chart, PokemonType.ELECTRIC to null, "not-a-real-ability") + assertEquals(withoutAbility, withUnknownAbility) + } + + @Test + fun `computeCompositeScore does not count a weakness the candidate's own ability immunizes away`() { + val teammate = buildMember("Teammate", PokemonType.NORMAL to null) + val context = teamScoringContext(chart, listOf(teammate)) + + val candidateWithoutAbility = buildMember("Pikachu", PokemonType.ELECTRIC to null) + val candidateWithLevitate = buildMember("Pikachu", PokemonType.ELECTRIC to null, ability = "levitate") + + val withoutAbility = computeCompositeScore(chart, candidateWithoutAbility, context, emptySet()) + val withLevitate = computeCompositeScore(chart, candidateWithLevitate, context, emptySet()) + + assertTrue(withoutAbility.newWeaknesses.contains(PokemonType.GROUND)) + assertFalse(withLevitate.newWeaknesses.contains(PokemonType.GROUND)) + assertTrue(withLevitate.compositeScore > withoutAbility.compositeScore) + } + + @Test + fun `teamScoringContext does not attribute a weakness to a teammate whose ability immunizes it`() { + val teammateWithoutAbility = buildMember("Zapdos", PokemonType.ELECTRIC to null) + val teammateWithLevitate = buildMember("Zapdos", PokemonType.ELECTRIC to null, ability = "levitate") + + val contextWithoutAbility = teamScoringContext(chart, listOf(teammateWithoutAbility)) + val contextWithLevitate = teamScoringContext(chart, listOf(teammateWithLevitate)) + + assertTrue(contextWithoutAbility.otherWeaknessMap.containsKey(PokemonType.GROUND)) + assertFalse(contextWithLevitate.otherWeaknessMap.containsKey(PokemonType.GROUND)) + + // A second Electric-type candidate shares that same single Ground weakness (weaknesses() + // for a plain Electric type is exactly [GROUND] in this fixture chart). Against the + // ability-less teammate, that weakness is aggravated (a real shared exposure); against the + // Levitate teammate, whose own Ground weakness the ability already removed, it is only new + // — aggravating costs 1.0, a brand-new weakness only 0.5 (NEW_WEAKNESS_PENALTY / + // AGGRAVATED_WEAKNESS_PENALTY), so the two scores must differ. Before the finding 6 fix, + // both contexts would have counted the teammate's weakness and wrongly agreed. + val candidate = buildMember("Raichu", PokemonType.ELECTRIC to null) + val aggravated = computeCompositeScore(chart, candidate, contextWithoutAbility, emptySet()) + val newOnly = computeCompositeScore(chart, candidate, contextWithLevitate, emptySet()) + + assertTrue(aggravated.aggravatedWeaknesses.contains(PokemonType.GROUND)) + assertTrue(newOnly.newWeaknesses.contains(PokemonType.GROUND)) + assertTrue(newOnly.compositeScore > aggravated.compositeScore) + } +} diff --git a/docs/implementation-decisions.md b/docs/implementation-decisions.md index c13f1df..950411f 100644 --- a/docs/implementation-decisions.md +++ b/docs/implementation-decisions.md @@ -808,8 +808,32 @@ findings not yet acted on. resource (`suggestions_exclude_legendaries`). See the corrected finding 4 in `docs/post-migration-review.md` for the full record, including the quoted spec text. -- **Finding 6 (abilities ignored by suggestion/generator scoring) — not - yet acted on.** Left for its own follow-up commit since it changes the - composite score's output, not just its performance or a missing - feature — it needs updated test expectations alongside it, not bundled - with an unrelated fix. +- **Finding 6 (abilities ignored by suggestion/generator scoring) — + `weaknesses()` gained an `ability` parameter, a real spec change.** + `weaknesses(chart, types)` never took an ability, unlike + `sharedWeaknessCounts`/`defensiveProfile` in `CoverageEngine.kt`, which + always honoured it — so a team's Analysis screen could show a member + as immune to a type (via the coverage grid) while the Suggestions + section, on the same screen, still penalized a candidate for + "aggravating" that exact weakness. Added `ability: String? = null` to + `weaknesses()`, threaded `candidate.ability` and each `otherMembers` + member's `.ability` through `computeCompositeScore`/ + `teamScoringContext`. Also fixed `memberFromEntry` (dropped from Phase + 4 as `ability = null` for every catalogue-derived candidate) to carry + `PokemonEntry.defaultAbility`, so a candidate is scored with the + ability it will actually have once applied, not always with none — + this let `TeamGenerator.Candidate` drop its own separate `ability` + field entirely (both construction paths already produce a `member` + whose own `ability` is correct, so the "pick the ability to apply" + step this field existed for is a no-op now). This *is* a spec change: + composite scores now differ from the ported TypeScript baseline for + any candidate or team member with a scoring-relevant ability (the 14 + entries in `AbilityEffects.kt`'s `ABILITY_EFFECTS`). Every existing + fixture in `TestFixtures.kt`/`SuggestionEngineTest.kt`/ + `TeamGeneratorTest.kt` has `defaultAbility = null` and builds every + `TeamMember` with no explicit `ability` either, so this change is + invisible to every existing exact-score/exact-ranking assertion — a + new `ScoringTest.kt` exercises the new behavior directly (Levitate + removing a candidate's own Ground weakness; a teammate's Levitate + changing a shared candidate weakness from "aggravated" to merely + "new"). diff --git a/docs/post-migration-review.md b/docs/post-migration-review.md index 0f9ea54..5f95692 100644 --- a/docs/post-migration-review.md +++ b/docs/post-migration-review.md @@ -28,15 +28,27 @@ maths is wrong. The defects are all **at the seams**: one port that silently changed a sort into a non-deterministic one, two engines invoked on the main thread, -and a slice of the suggestion UI that was never carried over. None of these -is reachable by the existing unit tests, for a structural reason worth -stating plainly: the test pools are 5–20 entries, the production pool is -several hundred, and no test asserts anything about threading. - -Separately: **0 of 59 items in [`docs/test-plan.md`](test-plan.md) are -ticked.** Every phase is marked ✅ in `CLAUDE.md` on the strength of CI and -unit tests alone. Three of the five findings below are exactly the kind a -single pass on a real device would have caught immediately. +a shipped stepper that placed nothing, redundant per-candidate rework, and +one place where ability was honoured on one side of a screen and ignored +on the other. Most of these are not reachable by the existing unit tests, +for a structural reason worth stating plainly: the test pools are 5–20 +entries, the production pool is several hundred, and no test asserted +anything about threading before this review added one. + +Separately: **0 of 59 items in [`docs/test-plan.md`](test-plan.md) were +ticked** at the time of this review. Every phase is marked ✅ in +`CLAUDE.md` on the strength of CI and unit tests alone. Findings 1, 2 and +5 below are exactly the kind a single pass on a real device would have +caught immediately — that gap has not been closed by this review or the +fixes that followed it; it is still a real gap. + +**All six findings below have since been fixed**, each in its own commit +on top of this review, including one correction (finding 4, see below) to +a claim in the original version of this document that turned out to be +wrong before it was acted on. The findings are kept below as written at +the time, not rewritten to look correct in hindsight, because the record +of what was found and how it was fixed is more useful than a document that +only ever describes a codebase with no known problems in it. ## Findings @@ -252,20 +264,22 @@ updated test expectations in the same commit. Worth doing; do it on its own. ## Plan -Ordered by risk removed per unit of work. +Ordered by risk removed per unit of work. **All six items below are now +done** — this section is kept as the reasoning behind the order they were +done in, not a to-do list. **Now — correctness** -1. Finding 1: memoize the score in `regenerateSlot`, plus the large-pool +1. ✅ Finding 1: memoize the score in `regenerateSlot`, plus the large-pool regression test. One-line fix, removes a crash. -2. Finding 5: decide and act on `customSlots` — implement, or remove the - stepper. Currently misleading either way. +2. ✅ Finding 5: decide and act on `customSlots` — implement, or remove the + stepper. Currently misleading either way. (Implemented.) **Next — responsiveness** -3. Finding 2: move both engines off the main thread; add the generator's +3. ✅ Finding 2: move both engines off the main thread; add the generator's progress indicator. -4. Finding 3: hoist the per-team precomputation out of the candidate loop. +4. ✅ Finding 3: hoist the per-team precomputation out of the candidate loop. Doing 4 before 3 is tempting and wrong: getting the work off the main thread is what fixes the jank, and the optimisation is then a bonus rather @@ -273,20 +287,26 @@ than load-bearing. **Then — parity and accuracy** -5. Finding 4 (corrected): add the solid-coverage message; clear the orphan - string. The type filter, random mode and 10-card cut are **not** part - of this — see the correction in finding 4 itself. -6. Finding 6: thread abilities through the scoring path, as a documented +5. ✅ Finding 4 (corrected): add the solid-coverage message; clear the + orphan string. The type filter, random mode and 10-card cut are **not** + part of this — see the correction in finding 4 itself. +6. ✅ Finding 6: thread abilities through the scoring path, as a documented spec change. **Underneath all of it** -7. Work `docs/test-plan.md` on a real device. 59 unchecked items is the +7. ⬜ Work `docs/test-plan.md` on a real device. 59 unchecked items is the single largest gap in confidence in this repository, and findings 1, 2 - and 5 are all things a first run-through would have surfaced. -8. Add the two test shapes that would have caught these: an engine test at - production pool scale (several hundred entries), and a ViewModel test - asserting the engines are not invoked on the collecting dispatcher. + and 5 are all things a first run-through would have surfaced. **Still + not done** — nothing in this review or its fixes ran on a device or an + emulator; see "What was not done" above and the same caveat repeated on + every fix commit. +8. ✅ Add the two test shapes that would have caught these: an engine test + at production pool scale (several hundred entries — `TeamGeneratorTest`'s + `largeTiedScorePool`), and a ViewModel test asserting the engines are + not invoked on the collecting dispatcher (`AnalysisViewModelTest`'s + `coverage is computed without ever advancing the Main test dispatcher`, + `SurpriseMeViewModelTest`'s `isGenerating` transition test). ## Areas swept, no defects found diff --git a/docs/test-plan.md b/docs/test-plan.md index a3f3cae..f854275 100644 --- a/docs/test-plan.md +++ b/docs/test-plan.md @@ -277,6 +277,20 @@ None yet. "solid coverage" message when every displayed suggestion has zero gain; also removed one genuinely orphaned string resource (`suggestions_exclude_legendaries`, referenced by no composable). +- **Fixed 2026-09-05.** `weaknesses()` (shared by suggestions and the + generator) never took an ability, unlike the coverage grid's own + `sharedWeaknessCounts`/`defensiveProfile` — found by code review + (`docs/post-migration-review.md`, finding 6), not by manual testing. + A team member's ability could remove a weakness on the coverage grid + while the Suggestions section, on the same screen, still counted that + weakness as "aggravated" by a candidate. `weaknesses()` now takes an + `ability` parameter, threaded through from both the candidate and every + team member scored against; `memberFromEntry` also now gives a + catalogue candidate its real default ability instead of always `null`. + This changes composite scores for any team/candidate with a + scoring-relevant ability — verify by hand: give a team member Levitate, + then check that a Ground-weak suggestion candidate is no longer marked + as aggravating that weakness. ## Phase 5 — Showdown import/export, settings and local backup From c9c17354b5240077d79b6b2a3f95afe578fb6bb8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 07:41:47 +0000 Subject: [PATCH 8/8] Fix CI: remove a flaky test, not a production bug CI failed on every commit from e9796d2 onward (findings 2, 3, 4, 6), all with the same single failure: SurpriseMeViewModelTest > isGenerating is true immediately after generate() and false again once it completes FAILED java.lang.AssertionError at SurpriseMeViewModelTest.kt:92 Confirmed by reading the job logs for the first and last failing runs: every other test passed in both, so this was one bad test, not a regression introduced by findings 3/4/6's changes. Root cause: the test asserted vm.uiState.value.isGenerating synchronously on the line right after calling generate(), on the assumption that the launched coroutine "couldn't have finished yet." That assumption was wrong. generate() launches directly on Dispatchers.Default - a real thread pool, not gated by the test's StandardTestDispatcher/TestCoroutineScheduler at all (that was the whole point of finding 2's fix: get real work off the main thread). Against the ~10-entry mock pool this test file uses, the background computation can complete and flip isGenerating back to false before the test's very next JVM instruction runs. A race, and evidently one CI loses close to 100% of the time in this environment, not an occasional flake. Removed the test rather than patching it: there is no reliable way to observe that transient true state from outside without an injectable dispatcher for the background work, and finding 2's own decision record (implementation-decisions.md) already explains why that was considered and rejected. The behavior it was trying to demonstrate (the coroutine eventually finishes and updates state) is still covered by the existing `generate fills the result from the pool and keeps a locked anchor first` test. Updated the corresponding claim in docs/post-migration-review.md's Plan section (item 8), which had cited this test by name. No production code changed in this commit. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_018zXVpxEbSWadqKyntXTXD6 --- .../ui/surprise/SurpriseMeViewModelTest.kt | 14 ------------- docs/post-migration-review.md | 21 ++++++++++++++++--- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/app/src/test/java/com/marcogn/coverdex/ui/surprise/SurpriseMeViewModelTest.kt b/app/src/test/java/com/marcogn/coverdex/ui/surprise/SurpriseMeViewModelTest.kt index 42920cb..650c305 100644 --- a/app/src/test/java/com/marcogn/coverdex/ui/surprise/SurpriseMeViewModelTest.kt +++ b/app/src/test/java/com/marcogn/coverdex/ui/surprise/SurpriseMeViewModelTest.kt @@ -88,20 +88,6 @@ class SurpriseMeViewModelTest { assertTrue(state.result.size <= 6) } - @Test - fun `isGenerating is true immediately after generate() and false again once it completes`() = runTest(mainDispatcherRule.dispatcher) { - val vm = viewModel() - vm.uiState.first { it.chart != null } - - vm.generate() - // isGenerating flips synchronously, before generate() even returns — no suspension - // needed to observe it, unlike the background computation itself. - assertTrue(vm.uiState.value.isGenerating) - - val state = vm.uiState.first { !it.isGenerating } - assertTrue(state.result.isNotEmpty()) - } - @Test fun `regenerateSlot only changes the targeted non-locked slot`() = runTest(mainDispatcherRule.dispatcher) { val vm = viewModel() diff --git a/docs/post-migration-review.md b/docs/post-migration-review.md index 5f95692..2e54878 100644 --- a/docs/post-migration-review.md +++ b/docs/post-migration-review.md @@ -304,9 +304,24 @@ than load-bearing. 8. ✅ Add the two test shapes that would have caught these: an engine test at production pool scale (several hundred entries — `TeamGeneratorTest`'s `largeTiedScorePool`), and a ViewModel test asserting the engines are - not invoked on the collecting dispatcher (`AnalysisViewModelTest`'s - `coverage is computed without ever advancing the Main test dispatcher`, - `SurpriseMeViewModelTest`'s `isGenerating` transition test). + not invoked on the collecting dispatcher + (`AnalysisViewModelTest`'s `coverage is computed without ever advancing + the Main test dispatcher`). A second such test on `SurpriseMeViewModel` + was written and then removed — CI caught it as flaky, not the + production code: it asserted `isGenerating.value` synchronously right + after calling `generate()`, assuming the launched coroutine couldn't + have finished yet, but `Dispatchers.Default` is a real thread pool the + test's `StandardTestDispatcher` does not gate, and the tiny mock pool + in that test file finishes fast enough to occasionally (in CI, + consistently) flip `isGenerating` back to `false` before the very next + line ran. Removed rather than patched: there is no reliable way to + observe that transient state from outside without adding an injectable + dispatcher for the background work, which finding 2's own decision + record (`implementation-decisions.md`) already explains was considered + and rejected. The behaviour it was trying to test (the coroutine + eventually finishes and updates state) is still covered by + `generate fills the result from the pool and keeps a locked anchor + first`. ## Areas swept, no defects found