Post-migration review: fix all 6 findings in coverage/suggestion/generator path - #34
Merged
Merged
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018zXVpxEbSWadqKyntXTXD6
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018zXVpxEbSWadqKyntXTXD6
Marcogn
changed the title
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<TeamMember> = 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018zXVpxEbSWadqKyntXTXD6
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018zXVpxEbSWadqKyntXTXD6
…ding 3) computeCompositeScore took otherMembers: List<TeamMember> 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018zXVpxEbSWadqKyntXTXD6
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018zXVpxEbSWadqKyntXTXD6
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018zXVpxEbSWadqKyntXTXD6
Marcogn
changed the title
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018zXVpxEbSWadqKyntXTXD6
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Post-migration code review of the coverage and suggestion/generator path, plus a fix for every finding it turned up — one finding per commit, 7 commits total.
docs/post-migration-review.mdA code-level audit against
docs/plan/native-spec.mdand the phase plans.legacy-web/was deleted in3e11ea8but survives in the log, so every Kotlin port was diffed against its TypeScript original recovered withgit show 3e11ea8^:legacy-web/...— the same oracle Phase 6 used, applied independently.The ported maths is faithful.
CoverageEngine.kt,AbilityEffects.kt,Scoring.ktandSuggestionEngine.ktmatch their originals function for function. The defects were all at the seams, and none was reachable by the existing unit tests (test pools are 5–20 entries against a production pool in the high hundreds; no test asserted on threading before this review added one).regenerateSlot's sort selector re-rolled its random noise on every comparison, breakingComparator's contract — TimSort threw aboveMIN_MERGE, and the "top 5" was arbitrary even when it didn'tDispatchers.Main.immediatecomputeCompositeScoreredid per-team work once per candidate instead of once per team0 of 59 items in
docs/test-plan.mdwere ticked at review time — findings 1, 2 and 5 are exactly the kind a single on-device pass would have caught. That gap is not closed by this PR: nothing here ran on a device or emulator (no Android SDK in the review sandbox), and manual verification perdocs/test-plan.mdremains outstanding.The first draft of this review claimed the Suggestions panel was missing the PWA's type-filter chips, a "Best coverage"/"Random" mode toggle, and showed 5 cards where the PWA showed 10 — based on diffing against the legacy React components alone, without checking this rewrite's own spec first.
docs/plan/native-spec.md's "Suggestion engine" section says, verbatim, "Return the top 5 bygain" for both addition and replacement mode, and specifies neither of the other two items —SuggestionFilters.kt's own doc comment already said as much. Five cards is the spec for this rewrite, not a shortfall; none of that was implemented. What survived, independent of the retracted items: a "solid coverage" message (shown when every displayed suggestion has zero gain) and removing one genuinely orphaned string resource. Full correction in the review doc andimplementation-decisions.md.The fixes, one per commit
computeScore's random noise was re-evaluated insidesortedByDescending's selector instead of once per candidate — non-transitive comparator, TimSort throws aboveMIN_MERGE. Fixed by scoring once, sorting on the stored value. New regression test: 300-entry same-typed pool (worst case for the bug) across 50 seeds.TeamGeneratornever readGeneratorConstraints.customSlots, so the stepper consumed budget and placed nothing. Implemented: both generator entry points now take acustomslist and treat it as a reserved category via a smallCandidatewrapper.AnalysisViewModel's pipeline now ends in.flowOn(Dispatchers.Default);SurpriseMeViewModel.generate()/regenerateSlot()setisGeneratingsynchronously then run entirely onDispatchers.Default. Surprise Me gained a progress indicator and disables its actions while generating.computeCompositeScoreintoTeamScoringContext, built once per team instead of once per candidate (wascandidates × 6in replacement mode).weaknesses()now takes an ability parameter, threaded through both the candidate and every team member.memberFromEntrynow gives a candidate its real default ability instead of alwaysnull. NewScoringTest.ktexercises the new behavior directly.Every fix commit documents, in its own message and in
implementation-decisions.md, why every existing test either still passes unchanged or was updated with the reasoning spelled out (e.g. why oneSurpriseMeViewModelTestpredicate needed to change onceregenerateSlotbecame async).Verification caveat, repeated because it applies to every commit above: this sandbox has no Android SDK (
ANDROID_HOMEempty, nosdkmanager, confirmed by running./gradlew testDebugUnitTestand watching it fail at SDK resolution before compiling), so none of this was verified by a local build or test run. CI is subscribed and watched, and is the actual verification for all of it.🤖 Generated with Claude Code
https://claude.ai/code/session_018zXVpxEbSWadqKyntXTXD6