Phase 1 Code Review — Adversarial Findings #

Scope: full codebase as-is before any UI phase work.
Audience: high-level explanations for the author + actionable agent instructions.
Findings ranked: Bugs → Dead/Unused Code → Architecture → Testing Gaps → Simplification

---

1. Out-of-Bounds Array Access in update_next_sphere_hud #

Severity: Bug / Panic

What's happening: TIER_COLORS is a 9-element array (indices 0–8). In hud.rs:341, the index is clamped with .min(9) instead of .min(8).

// hud.rs line 341
let idx = (queue.next as usize).saturating_sub(1).min(9); // ← should be .min(8)
let color = TIER_COLORS[idx]; // panics if idx == 9

If queue.next is ever 11 or higher (which can't happen today since dispensed tiers cap at 5, but becomes possible the moment you expand the dispenser), TIER_COLORS[9] panics at runtime. Compare to material_for_tier in visuals.rs:74 which correctly uses .min(8). One copy got the right number; the other didn't.

Agent fix: Change hud.rs:341 to .min(8).

---

2. InsideLauncher Is Never Inserted — Component Is Dead Code #

Severity: Bug (silent logic gap) + dead code

What's happening: InsideLauncher is defined, tested, and used as a query filter in three separate systems (check_loss_condition, detect_collisions, update_launcher_aiming), but it is never inserted on any entity in gameplay code. The filters are therefore no-ops. This was presumably meant to mark the physics sphere while it's being aimed at the launcher line, but the design was changed to use a separate visual-only LauncherPreview entity instead — and InsideLauncher was never removed.

Concrete consequence: the exclusion filter in check_loss_condition has no effect. Any sphere spawned at launcher_z would start triggering the loss timer immediately (the 0.5s grace period rescues it in practice, but only accidentally).

Agent fix:

• Remove the InsideLauncher struct from game_state.rs.
• Remove all Without<InsideLauncher> filter clauses from query tuples in check_loss_condition, detect_collisions, and update_launcher_aiming.
• Remove the launcher-exclusion test test_loss_condition_launcher_exclusion (it tests a component that no longer exists).

---

3. despawn_recursive_custom Is Duplicated Across Two Modules #

Severity: Code smell / maintenance hazard

What's happening: Identical private functions are copy-pasted verbatim in game_state.rs:207–218 and hud.rs:467–478. Any bug fix or behavioral change must be applied twice.

// Exact same code in both files:
fn despawn_recursive_custom(commands: &mut Commands, entity: Entity, children_query: &Query<&Children>) {
    if let Ok(children) = children_query.get(entity) {
        for child in children.iter() {
            despawn_recursive_custom(commands, child, children_query);
        }
    }
    commands.entity(entity).despawn();
}

Bevy's EntityCommands::despawn() in current Bevy 0.18 despawns only the entity itself — children are orphaned unless removed separately. The custom recursive walk is legitimate. But it should live in one place.

Agent fix:

• Create src/utils.rs (or add a public function to an existing module) with pub fn despawn_recursive(commands: &mut Commands, entity: Entity, children_query: &Query<&Children>).
• Replace both call sites with the shared function.
• Add mod utils; to main.rs.

---

4. material_for_tier Takes a _colorblind Parameter It Never Uses #

Severity: Unused code / misleading API

What's happening: The function signature is:

// visuals.rs:70
pub fn material_for_tier(tier: u8, _colorblind: bool, tier_mats: &TierMaterials) -> Handle<StandardMaterial> {

The _colorblind argument is silently ignored (the _ prefix is a Rust convention meaning "intentionally unused"). Every call site passes false or cb but nothing changes. The colorblind experience today is handled entirely through 2D text label visibility, not material switching.

Keeping this parameter creates a false promise: callers think they can request a colorblind material variant; they can't.

Agent fix:

• Remove the _colorblind: bool parameter from material_for_tier.
• Update the two call sites: on_sphere_added (visuals.rs:243) and update_preview_material (visuals.rs:294).
• Document this decision with a comment if colorblind material variants are planned for a future phase.

---

5. ColorblindMode Is Registered Twice #

Severity: Minor redundancy

What's happening:

// main.rs:30
.insert_resource(game_state::ColorblindMode::default())

// visuals.rs VisualPlugin::build:
.init_resource::<ColorblindMode>()

init_resource is a no-op when the resource already exists, so the VisualPlugin call silently does nothing. This is harmless but confusing: a reader of VisualPlugin would assume it owns the resource, and a reader of main.rs would assume the same.

Agent fix: Remove the init_resource::<ColorblindMode>() line from VisualPlugin::build in visuals.rs. Ownership of global game resources belongs in main.rs.

---

6. update_launcher_preview_visuals Is Outside the InGame State Guard #

Severity: Inconsistency / fragility

What's happening: In LauncherPlugin::build, three systems are correctly gated on AppState::InGame:

(update_launcher_aiming, check_launcher_obstructions, handle_launch_input)
    .chain()
    .run_if(in_state(AppState::InGame)),
update_launcher_preview_visuals,   // ← runs in ALL states

update_launcher_preview_visuals compensates by manually checking the state inside its body and hiding itself when not in-game. This works, but it means the system runs every frame even during menus and game-over screens, and it reads four queries + a resource every frame to do nothing. It also means the logical gate is expressed in two different places.

Agent fix: Move update_launcher_preview_visuals inside the .chain() with the other systems and add it to the .run_if(in_state(AppState::InGame)) guard. Remove the manual is_in_game check inside the function body.

---

7. animate_merged_spawns Duplicates Its Own Loop Body #

Severity: Simplification

What's happening: The function contains two nearly-identical loops — one over visual_query (mesh transforms) and one over label_query (text transforms) — that compute the same t/scale value from the same MergeCooldown timer:

// Repeated twice, once per query type:
if let Ok(cooldown) = cooldown_query.get(parent) {
    let t = (elapsed / duration).clamp(0.0, 1.0);
    let scale = 0.8 + t * 0.2;
    transform.scale = Vec3::splat(scale);
} else {
    if transform.scale != Vec3::ONE { transform.scale = Vec3::ONE; }
}

animate_fulfilling_spheres has the same structure.

Agent fix: Extract a helper:

fn compute_merge_scale(cooldown: &MergeCooldown) -> f32 {
    let t = (cooldown.timer.elapsed_secs() / cooldown.timer.duration().as_secs_f32()).clamp(0.0, 1.0);
    0.8 + t * 0.2
}

Then unify both query loops in animate_merged_spawns by looking up the scale once per parent entity and applying it to both the mesh child and the label. Same refactor for animate_fulfilling_spheres.

---

8. O(N²) Occlusion Check in update_labels_screen_position #

Severity: Performance / future scalability

What's happening: For every label (one per sphere), the function does a full linear scan over all other spheres to determine if the label is occluded:

// visuals.rs:346
for (other_entity, other_transform, other_sphere) in sphere_query.iter() { ... }

With N spheres on the board, this is O(N²) work per frame. At the current scale (max ~15–20 spheres before game-over), this is not a performance problem today. But it's the single most expensive Update system and will be the first thing to profile badly when sphere counts grow.

The analytical ray–sphere approach is elegant; the O(N²) structure is not.

Agent fix (Phase 1 — cleanup without rearchitecting):

• Cache the sphere positions in a Vec before the outer loop to avoid repeated ECS iteration overhead.
• Add a comment marking this as the known hotspot.

Agent fix (Phase 2 — correct approach):

• Replace the occlusion check with a simple depth-sort: label the sphere whose screen-space depth (NDC z) is smallest (closest to camera); skip labels behind others. This can be done in one O(N log N) sort pass.

COMMENT: I'm not sure abouot this one, because of the risk of a sphere that's large blocking multiple smaller ones.

---

9. check_order_fulfillment Weight Formula Is Opaque #

Severity: Readability / maintenance hazard

What's happening: The tier selection logic works correctly, but the construction of tier-7's probability as an inline expression is hard to audit:

// game_state.rs:177
} else if roll < w5 + w6 + (100 - w5 - w6 - w8) {
    7

100 - w5 - w6 - w8 is tier 7's weight (the remainder), but it's never named, and the three-level if/else chain obscures the boundary math. The comment block above it accurately describes the intent — but anyone editing the weights must mentally re-derive that none of the four weights sum past 100.

Additionally, (60 as u32) is an unnecessary cast; 60u32 is idiomatic Rust.

Agent fix:

let w8 = (score.completed_orders * 10).min(40);
let w6 = 60u32.saturating_sub(score.completed_orders * 10).max(15);
let w5 = 5u32;
let w7 = 100 - w5 - w6 - w8;  // ← named remainder

let roll = rand::random_range(0..100u32);
active_order.target_tier = if roll < w5 {
    5
} else if roll < w5 + w6 {
    6
} else if roll < w5 + w6 + w7 {
    7
} else {
    8
};

---

10. Visibility Guard Pattern Repeated ~15 Times #

Severity: Simplification

What's happening: Throughout visuals.rs and launcher.rs, the same guarded-set pattern appears repeatedly:

if *visibility != Visibility::Hidden {
    *visibility = Visibility::Hidden;
}

This idiom exists to avoid triggering change detection when the value isn't actually changing (a valid Bevy optimization). But writing it inline every time creates visual clutter and is easy to write wrong (e.g., checking Hidden but setting Visible by accident).

Agent fix: Add two helper functions (e.g., in utils.rs):

pub fn set_visibility(vis: &mut Visibility, target: Visibility) {
    if *vis != target { *vis = target; }
}

Then replace all inline guard patterns with set_visibility(&mut visibility, Visibility::Hidden).

---

11. Testing Gaps #

11a. test_weighted_dispenser_distribution Tests a Tier That Cannot Exist #

core_math.rs:79 asserts tier >= 1 && tier <= 5, but get_random_dispensed_tier only returns 1–4 (_ => 4). Tier 5 is unreachable. The bounds check in the test is too wide and will silently accept a broken version of the function that returns 5.

Agent fix: Change the assert to tier >= 1 && tier <= 4.

11b. No Test for spill_spheres #

spill_spheres in physics.rs is the system that unlocks Y-translation and rotation when a sphere crosses launcher_z. It has no unit test. This is one of the trickier systems (depends on the Rapier context being present) but the axis-unlocking behavior is testable at the component level without a full physics simulation.

Agent test sketch:

• Spawn a sphere with full LockedAxes at z = 0, then teleport it to z > launcher_z. Run one update. Assert LockedAxes is now empty (or no longer present).

11c. No Test for Merge-Block During Active Cooldown #

handle_launch_input blocks firing when !merge_cooldowns.is_empty(). The existing test_launch_cooldown_blocking tests the timer-based cooldown but not the merge-cooldown gate. A sphere mid-merge should prevent a launch even if the cooldown timer has expired.

Agent test sketch:

• Set up an app with handle_launch_input, advance time past 0.8s cooldown.
• Spawn an entity with MergeCooldown.
• Press left-click. Assert no sphere is spawned.

11d. No Test for check_order_fulfillment Timer Reset Behavior #

The fulfillment timer is stored in ActiveFulfillment and reset via fulfillment.timer.reset() when a new match is found. There is no test that verifies re-triggering: if the fulfilling entity is despawned externally (e.g., via a merge) while fulfillment is in progress, fulfillment.entity still holds a stale handle and commands.entity(entity).despawn() will silently no-op in Bevy (despawning an already-despawned entity). The score update will still fire. This should be guarded.

Agent fix: Before calling commands.entity(entity).despawn() in check_order_fulfillment, check sphere_query.get(entity).is_ok() (or use commands.get_entity(entity)) and skip the score update if the entity no longer exists.

---

12. Fulfilling Spheres Bypass Distance-Merge Detection But Not Collision Detection #

Severity: Edge-case logic gap

What's happening: check_distance_merges excludes Fulfilling spheres from its proximity scan (the Without<Fulfilling> filter). But detect_collisions does not exclude Fulfilling spheres — only spheres Without<InsideLauncher> and Without<MergeCooldown>. If a free sphere collides with a fulfilling sphere of the same tier during the 1.2s fulfillment window, detect_collisions will fire a MergeEvent for the fulfilling entity.

resolve_merges will then try to despawn the fulfilling sphere via DespawnDelay, which happens after check_order_fulfillment's despawn also runs. Double-despawn in Bevy is a no-op, but the score is incremented once (by the fulfillment timer) and once more (by the merge points), and a new upgraded sphere is spawned from a sphere that was supposed to disappear as an order completion.

Agent fix: Add Without<crate::game_state::Fulfilling> to the detect_collisions sphere query filter, matching the existing guard in check_distance_merges.

---

13. Architecture: Visuals Are Tangled Into Physics Spawn #

Severity: Architecture (future-phase hazard)

What's happening: spawn_sphere_entity in physics.rs adds Visibility::default() to the physics entity:

// physics.rs:84
Visibility::default(),

This is a rendering concern living in the physics module. It works because Bevy requires Visibility for hierarchy propagation, but it's the thin end of a wedge that will make headless testing harder (all physics tests must either carry or ignore rendering components).

More structurally: the on_sphere_added observer in visuals.rs is the correct decoupling point — visuals react to physics events. But spawn_sphere_entity reaching into rendering concepts (via Visibility) blurs that line.

Agent fix for Phase 1:

• Remove Visibility::default() from spawn_sphere_entity.
• Add it to the on_sphere_added observer via commands.entity(entity).insert(Visibility::default()) if Bevy requires it for 2D overlay label positioning.
• Audit whether it's actually needed (labels use a separate root entity; the sphere mesh is a child spawned by the observer, so the parent may not need Visibility at all).

---

14. Win Condition Silently Swallowed if State Resource Is Absent #

Severity: Logic gap / silent failure

What's happening: resolve_merges in physics.rs declares its state resource as optional:

// physics.rs:165
mut next_state: Option<ResMut<NextState<crate::game_state::AppState>>>,

This means if AppState is not registered in the app (which is the case in every physics unit test that uses PhysicsPlugin directly), triggering the Tier 9 win condition does nothing — no panic, no warning, just a silent if let Some(ref mut ns) = next_state { ... } branch that is never taken.

The Option was likely added to make tests easier: PhysicsPlugin can be added to a test app without also wiring up the full state machine. But it trades test convenience for a production guarantee. If AppState were ever accidentally unregistered, or if a future refactor moves physics into a context where the state resource isn't present, the win condition would vanish without any compile-time or runtime signal.

Agent fix:

• Make the resource required: change Option<ResMut<NextState<AppState>>> to ResMut<NextState<crate::game_state::AppState>> in the system signature.
• In the tests that use PhysicsPlugin and exercise merges (test_collision_midpoint_merge, test_double_merge_safety, test_merge_momentum_conservation), add:

  app.add_plugins(bevy::state::app::StatesPlugin);
  app.init_state::<crate::game_state::AppState>();
  

  This is a few lines per test and keeps the production path honest.

---

15. hud.rs Has No Tests #

Severity: Testing gap

The HUD module (hud.rs) contains all UI layout, button interaction handlers, and score/order display logic. It currently has zero tests. The HUD is mostly presentational, but two behaviors are worth testing without a full rendering stack:

1. in_game_over_or_win_state run condition — ensure it correctly admits GameOver and Win but rejects MainMenu and InGame.
2. update_score_hud / update_order_hud text change guards — ensure text isn't rewritten every frame when the value hasn't changed (the != guard exists; it should be tested).

---

Summary Table #

#	File	Issue	Kind
1	hud.rs:341	TIER_COLORS indexed with .min(9) instead of .min(8) — potential panic	Bug
2	game_state.rs / queries	InsideLauncher never inserted; all exclusion filters are no-ops	Bug / Dead code
3	game_state.rs:207, hud.rs:467	despawn_recursive_custom copy-pasted verbatim	Duplication
4	visuals.rs:70	_colorblind parameter never used in material_for_tier	Dead code
5	main.rs:30, visuals.rs	ColorblindMode registered twice	Redundancy
6	launcher.rs plugin	update_launcher_preview_visuals outside InGame guard	Inconsistency
7	visuals.rs:519	Merge animation loop body duplicated for mesh vs. label	Duplication
8	visuals.rs:346	O(N²) occlusion check in label projection	Performance
9	game_state.rs:168–181	Order weight formula uses unnamed 100-w5-w6-w8 inline	Readability
10	visuals.rs, launcher.rs	if *vis != X { *vis = X; } pattern repeated ~15 times	Simplification
11a	core_math.rs:79	Test asserts tier ≤ 5 but function only returns 1–4	Test bug
11b	physics.rs	spill_spheres has no test	Test gap
11c	launcher.rs	Merge-cooldown launch block has no test	Test gap
11d	game_state.rs:149–154	Stale fulfillment entity not guarded before despawn	Logic gap
12	physics.rs:101	detect_collisions doesn't exclude Fulfilling spheres	Bug
13	physics.rs:84	Visibility::default() in physics spawn function	Architecture
14	physics.rs:165	Win condition uses Option<ResMut<NextState>> — silently swallowed if resource absent	Logic gap
15	hud.rs	Zero tests	Test gap