diff --git a/CHANGELOG.md b/CHANGELOG.md index 7da4379..bcfe2b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,62 @@ 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. +- **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. +- **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. +- **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. +- **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. +- **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. + Records six findings and an ordered remediation plan in + [`docs/post-migration-review.md`](docs/post-migration-review.md). + ## [2.0.0] - 2026-09-04 CoverDex is now a native Android app — the full six-phase rewrite 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..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 @@ -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 /** @@ -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,20 +68,28 @@ 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() } -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) +/** 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 + * `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 } @@ -111,6 +125,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 +140,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 +180,32 @@ 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) }!! - - val newMember = best.second.copy(ability = best.first.defaultAbility) - team.add(newMember) - usedSpecies.add(best.first.displayName.lowercase()) - - if (isLegendaryOrMythical(best.first)) legendaryMythicalCount++ - if (isStarter(best.first)) starterCount++ - if (isMega(best.first)) megaCount++ - if (isDynamax(best.first)) dynamaxCount++ + // 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, context, random) }!! + + team.add(best.member) + usedSpecies.add(best.member.speciesName.lowercase()) + + 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,40 +222,63 @@ 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] } + // 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 { entry -> entry to memberFromEntry(entry) } - .sortedByDescending { (_, member) -> computeScore(chart, member, otherMembers, random) } + .map { candidate -> candidate to computeScore(chart, candidate.member, context, 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 } 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..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, @@ -33,47 +39,64 @@ 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, m.ability)) { + 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) } val offensiveGain = newUnion.size - currentTeamCoverage.size val newlyCovered = newUnion.filter { it !in currentTeamCoverage } - 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 candWeaknesses = weaknesses(chart, candidate.types, candidate.ability) 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..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, ) @@ -120,8 +124,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 +140,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/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 591574b..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) - 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) - 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/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/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/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/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 2f84070..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,66 @@ 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 + // 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/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/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..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 @@ -97,10 +97,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 00a730d..950411f 100644 --- a/docs/implementation-decisions.md +++ b/docs/implementation-decisions.md @@ -701,3 +701,139 @@ 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 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 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 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) — + `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 new file mode 100644 index 0000000..2e54878 --- /dev/null +++ b/docs/post-migration-review.md @@ -0,0 +1,333 @@ +# 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, +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 + +### 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 — 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 + +`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. **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 + 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. (Implemented.) + +**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 (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 + single largest gap in confidence in this repository, and findings 1, 2 + 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`). 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 + +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. diff --git a/docs/test-plan.md b/docs/test-plan.md index 36bc53d..f854275 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. @@ -207,6 +211,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. @@ -216,10 +225,72 @@ 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 -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. +- **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. +- **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. +- **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. +- **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). +- **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