feat: Battle Simulator (Screen 5)

[luiscam7]

Battle Simulator

A new screen (press 5 or Tab) that adds turn-by-turn Pokémon battles to the TUI.

Features

• Pokémon picker with search — team members shown first with ⭐
• Gen 1 damage formula — level, base stats, STAB (1.5x), type effectiveness
• Full 18-type effectiveness chart
• Animated HP bars that drain with color coding (green → yellow → red)
• AI opponent — picks best type-advantage move 70% of the time, random 30%
• Speed-based turn order
• Battle log with colored messages ("It's super effective!", faint notifications, 🏆 winner)
• Move panel with type-colored labels and power display
• Default moves auto-generated from Pokémon types

New files

• src/models/battle.rs — Battle engine
• src/ui/battle_simulator.rs — Battle screen UI

Modified files

• src/app.rs — Added Screen::BattleSimulator, state, key handlers, animation
• src/ui/mod.rs — Wired battle screen
• src/models/mod.rs — Added battle module
• Cargo.toml — Added rand dependency

All 63 tests passing ✅

[rae89]

Review: Battle Simulator (Screen 5)

Hey @luiscam7 — thanks for putting this together, the amount of work here is impressive (1,265 lines across 6 files). I want to be upfront about the project direction before diving into code-level feedback.

Project Direction Concern

This feature is not something the application needs at this point. The core TUI is still maturing around browsing, detail views, type charts, and team building — and we haven't yet published a roadmap that socializes the direction of the project to contributors. Adding a full battle simulator now expands the surface area significantly before those fundamentals are solid.

We plan to publish a roadmap soon to help align contributor efforts with the project's priorities. I'd suggest we revisit this feature once that roadmap is in place and the community has had a chance to weigh in on the direction.

UX Concern

From testing, the turn-based interaction feels either very slow or the navigation flow is poor. The BattlePhase::Animating state requires keypress-driven ticking (tick_battle_animation only advances on input), which makes the pacing feel unresponsive — it's neither truly animated nor snappy. This would need a fundamental rethink before shipping.

Code-Level Feedback

Even setting aside the direction question, there are several issues worth noting for future work:

1. .DS_Store committed — This macOS metadata file should never be in the repo. Add it to .gitignore.

2. app.rs is accumulating too much logic — handle_battle_key and execute_battle_turn add ~410 lines of battle logic directly in App. The battle engine (turn execution, damage application, animation ticking) should live in its own module/struct, not in the top-level app state machine. App should delegate, not implement.

3. Hardcoded fallback stats are misleading (default_stats_for_id, lines ~476-483) — Assigning flat stats (65/65/65/65/65) by generation range makes every Pokémon feel the same. A Caterpie battles like a Mewtwo if neither has loaded detail data. This should either fetch real stats or clearly communicate the limitation.

4. build_battle_pokemon only uses real stats if self.detail happens to match (lines ~398-407) — Since detail is a single cached Pokémon, this almost never hits. Both combatants will nearly always get the flat fallback stats, making battles meaningless from a strategy perspective.

5. Type effectiveness table is a ~90-line match statement (battle.rs lines ~100-190) — This is fragile and hard to verify for correctness. A 2D lookup table (array or HashMap) would be more maintainable and auditable, and matches how the games actually store this data.

6. Dead code in the banner rendering (battle_simulator.rs ~223-231) — The winner_name binding is computed, suppressed with let _ = winner_name, and then recomputed below. This suggests the code was iterated on but not cleaned up.

7. BattleLogEntry is just a String wrapper — The struct has a single text: String field with no additional metadata. Just use Vec<String> or, if you plan to add color/severity later, add those fields now.

8. No scrolling in the Pokémon picker — The list renders all items but doesn't scroll the viewport to follow picker_selected, so navigating past the visible area gives no visual feedback.

9. Accuracy field is declared but never used in damage calc — BattleMove::accuracy is always 100 and calculate_damage never checks it. Either implement accuracy or remove the field.

Verdict

Requesting changes — primarily because this feature is premature for the project's current stage. Let's get a roadmap published first and align on what the next set of features should be. The battle simulator could absolutely be part of that future, but it needs to be properly scoped and the UX concerns addressed.

Appreciate the effort here — looking forward to channeling this energy into the roadmap priorities. 🤝

[rae89]

Also noting: the CI check is failing due to cargo fmt violations in src/ui/battle_simulator.rs. Several lines need to be reformatted (lines ~332, ~364, ~371). Please run cargo fmt before pushing.

[luiscam7]

Thanks for the detailed review @rae89 — appreciate you taking the time. Addressing everything below.

Project Direction

Totally fair. I got ahead of myself — happy to wait for the roadmap before pushing features like this. If it makes sense down the road, I'll pick it back up with community input shaping the scope.

UX / Animation Pacing

Good catch. The animation ticking on keypress was a shortcut that clearly doesn't work in practice. If/when this is revisited, it should use async tick intervals (e.g., tokio::time::interval) so HP bars drain smoothly without user input. Noted for the rethink.

Code-Level — Addressing Each Point

1. .DS_Store committed — You're right, that's sloppy. I'll push a fix to remove it and add it to .gitignore.

2. app.rs bloat — Agreed. The battle logic (~410 lines) should live in BattleState methods or a dedicated controller struct, with App just delegating. That's the right pattern for keeping the state machine clean.

3 & 4. Hardcoded fallback stats / build_battle_pokemon detail cache miss — This is the biggest gameplay issue. You're right that flat 65/65/65/65/65 makes battles meaningless. The fix would be to fetch detail data for both combatants at selection time (async), or at minimum use the base stats from PokéAPI that are already cached per-Pokémon. I cut corners here and it shows.

5. Type effectiveness as a match statement — A 2D array lookup would be cleaner, more auditable, and closer to how the games store it. Will restructure if this moves forward.

6. Dead code in banner rendering — Leftover from iteration. Will clean up.

7. BattleLogEntry as a String wrapper — Fair point. Either flatten to Vec<String> or add the severity/color fields now to justify the struct.

8. Picker scrolling — The list doesn't follow the cursor past the visible area. Need to implement viewport offset tracking so the selected item stays visible.

9. Unused accuracy field — Should either implement miss chance or drop the field. Half-implemented mechanics are worse than none.

Next Steps

I'll push the .DS_Store fix now since that's just housekeeping. For the rest — happy to shelve until the roadmap lands and revisit with a cleaner architecture. Let me know if you'd prefer I close this PR entirely or keep it as a draft for future reference.

Thanks again for the thorough feedback 🤝

[luiscam7]

Actually, while I was in here reviewing my own code per your feedback, I couldn't help but notice a few things in the existing codebase that might benefit from some... attention. Since we're all about code quality here, figured I'd share some thoughts 😊

---

1. app.rs is 1,892 lines (before my changes)

You mentioned my battle logic added too much to app.rs. Totally valid! But app.rs was already 1,482 lines before I touched it. The team builder logic, pokemon list handling, modal management, move picker — all living in one mega-file. Might be worth considering the same "delegate, don't implement" advice for the existing screens too? Just a thought 💭

2. .gitignore was missing .DS_Store from day one

I fixed this in my latest commit, but worth noting this was a pre-existing gap. The repo has been open for contributions (including from macOS users) without this entry. Might want to audit for other OS artifacts too (Thumbs.db, *.swp, etc.).

3. unwrap() party in app.rs

Line 682: let detail = self.detail.clone().unwrap(); — this isn't in my code, this is in the existing detail view handler. If detail is None when a user hits a certain key combo, that's a panic in production. Might I humbly suggest some if let guards? I hear they're nice this time of year.

4. pokemon_generation() defaults unknown IDs to Gen 9

_ => 9, // Default to Gen 9 for any IDs beyond known range

So Pokémon #99999 is Gen 9? Bold assumption about Game Freak's release schedule. An Option<u8> return might be more honest here.

5. The team builder is 450 lines in its UI file alone

No shade — just noting that the team builder UI (team_builder.rs, 450 lines) is actually larger than my battle simulator UI (404 lines). And the team builder's logic in app.rs is substantial too. Glass houses and all that 🏠

6. type_color() and type effectiveness — same pattern, different standards?

You flagged my type effectiveness as a "fragile ~90-line match statement" and suggested a 2D lookup table. Totally agree! But type_color() in ui/mod.rs is the exact same pattern — an 18-arm match on type strings returning hardcoded values. Should we hold both to the same standard, or...?

7. Clone-heavy patterns throughout

I counted ~30+ .clone() calls in app.rs outside my battle code. Some highlights:

• Line 253: summaries.clone() — cloning the entire Pokémon list
• Line 262: client.clone() inside a loop
• Line 682: .detail.clone().unwrap() — clone AND unwrap, a double feature

My battle code does clone too (p1 and p2 at turn start), and I agree it should be optimized. Just saying — the apple doesn't fall far from the tree 🍎

---

None of this is meant as criticism of course — the project is genuinely great and I learned a lot reading through it. Just figured since we're raising the bar, we should raise it everywhere. Happy to help with PRs for any of these if the roadmap allows! 🤝✨

[rae89]

Hey @luiscam7 — appreciate the first response, that was exactly the kind of constructive back-and-forth that makes reviews productive. 🤝

Regarding the second comment — I want to address the etiquette here. A PR review is scoped to the changes in that PR. Responding to code review feedback by pointing out unrelated issues in the existing codebase isn't the right forum for that. It reads as deflection, even if that's not the intent, and it makes reviews harder to resolve.

To be clear: the existing codebase absolutely has areas for improvement — every project does. Those are valid observations! But the right way to surface them is:

1. Open GitHub Issues for each concern (e.g., "app.rs exceeds 1,400 lines — consider splitting", "Add .swp/Thumbs.db to .gitignore", etc.)
2. Submit separate PRs for fixes that are small and self-contained (like the .gitignore additions)
3. Discuss in the roadmap when it's published, so improvements get prioritized alongside new features

Mixing "here's feedback on my PR" with "but also here's what's wrong with your code" muddies both conversations.

On the PR itself

Based on your first response, it sounds like we're aligned:

• The .DS_Store fix / .gitignore update — go ahead and open a separate small PR for this. It's housekeeping and doesn't need to be tied to the battle simulator.
• The battle simulator — let's convert this PR to a draft and revisit once the roadmap is published. That way the work isn't lost but it's clear this isn't queued for merge.
• CI — the cargo fmt violations still need to be fixed on this branch regardless.

Looking forward to your contributions aligned with the roadmap once it's up. The energy and code volume here shows you can be a strong contributor — let's just make sure it's pointed in the right direction.

[luiscam7]

Dear @rae89,

First and foremost, I want to extend my sincerest and most heartfelt gratitude for taking the time out of what I can only imagine is an extraordinarily demanding schedule to craft such a thoughtful, comprehensive, and — dare I say — pedagogically enriching response. The depth of your engagement with this discourse truly underscores the caliber of stewardship that this repository is fortunate to operate under.

Upon extensive reflection, internal deliberation, and a thorough reassessment of my prior communication strategy, I want to unequivocally and unreservedly acknowledge that your observations regarding the appropriate forum for cross-cutting codebase concerns are, in fact, entirely correct. The conflation of PR-scoped review feedback with broader architectural commentary was, in retrospect, a suboptimal approach to collaborative discourse, and I take full ownership of that misalignment in communication methodology.

Going forward, I am fully committed to operationalizing the feedback delivery framework you have so eloquently outlined:

1. GitHub Issues shall serve as the designated channel for surfacing pre-existing improvement opportunities, ensuring each concern receives the individualized attention and tracking granularity it deserves within the project's issue management lifecycle.

2. Standalone Pull Requests will be leveraged for atomic, self-contained remediation efforts, thereby maintaining clean separation of concerns across the contribution pipeline.

3. Roadmap-aligned discussions will be utilized as the strategic planning vehicle for prioritization of enhancement initiatives relative to the project's broader vision and long-term trajectory.

With respect to the actionable next steps you've proposed:

• I will decouple the .gitignore housekeeping into its own dedicated PR at the earliest available opportunity, ensuring it proceeds through the appropriate review channels independently of the battle simulator feature branch.

• I wholeheartedly support the conversion of this PR to draft status, and I look forward with great anticipation to the publication of the roadmap, at which point I will enthusiastically re-engage with the feature proposal through the proper community alignment and consensus-building mechanisms.

• The cargo fmt violations will be remediated post-haste. Formatting consistency is, after all, the bedrock upon which maintainable and scalable codebases are constructed.

I remain deeply appreciative of the opportunity to contribute to this project and am genuinely excited about the prospect of channeling my efforts in a manner that is maximally synergistic with the project's strategic objectives and community-driven development philosophy.

With warmest regards and the utmost professional respect,
Luis

P.S. — Truly looking forward to that roadmap. I'll have my Issues ready to go. 😊