# Code Review Findings

## Fixed

### 1. Camera and PlayerController input conflict
**Files:** `scripts/camera_controller.gd`, `nodes/player_controller.gd`

Both scripts used `_unhandled_input` with left mouse button. The camera used left click for dragging and never called `set_input_as_handled()`, so every left click also triggered unit selection/movement. Fixed with a drag threshold and click-on-release pattern.

---

## Outstanding

### 2. Unit lookup is fragile
**File:** `nodes/player_controller.gd:75`

`_get_unit_at()` iterates `get_parent().get_children()` to find units. This couples the controller to a specific scene tree layout and will degrade as the scene grows.

**Fix:** Use a group (`"units"`) or a dedicated unit registry so lookup doesn't depend on parent structure.

### 3. No pathfinding — movement can skip walls diagonally
**File:** `nodes/player_controller.gd:45-60`

Step-by-step movement picks one axis per frame. If the goal is diagonally past an L-shaped wall, the unit can path around it depending on approach angle since only the next immediate tile is checked.

**Fix:** Implement BFS/A* on the grid, or at minimum validate the full path before starting movement.

### ~~4. CombatMap extends CanvasItem instead of Node2D~~ (fixed)
**File:** `nodes/combat_map.gd:2`

Changed to extend `Node2D`.

### ~~5. Inconsistent naming convention~~ (fixed)
**File:** `nodes/combat_map.gd:16`

Renamed `tileCoords` to `tile_coords`.

### ~~6. unit_template is a string path, not a PackedScene~~ (fixed)
**File:** `scripts/test_map_generator.gd:7`

Changed to `@export var unit_template: PackedScene` and removed the `load()` call. **Note:** The scene must be assigned in the Godot inspector on the TestMapGenerator node.

### ~~7. No current_hp on units~~ (fixed)
**File:** `nodes/unit.gd`

Added `current_hp` variable, initialized to `max_hp` in `_ready()`.

### ~~8. Debug print left in production code~~ (fixed)
**File:** `scripts/test_map_generator.gd`

Removed the debug `print()` call.

### 9. tile_highlight.gd runs every frame unconditionally
**File:** `scripts/tile_highlight.gd:12-17`

`_process` updates position and alpha every frame even when the highlight is hidden. Could skip processing when invisible via `set_process(false)` on hide.

### 10. GridOverlay is wired but unused
**Files:** `scripts/grid_overlay.gd`, `nodes/combat_map.gd:6`

`highlight_map` reference exists but is never called from any game logic. Dead code at the moment.

---

## Priority

| Priority | Issue |
|----------|-------|
| ~~High~~ | ~~#1 Input conflict~~ (fixed) |
| High | #3 Wall-skipping movement |
| Medium | #2 Fragile unit lookup |
| ~~Medium~~ | ~~#6 String path instead of PackedScene~~ (fixed) |
| ~~Low~~ | ~~#4 CanvasItem base class~~ (fixed) |
| ~~Low~~ | ~~#5 Inconsistent naming~~ (fixed) |
| ~~Low~~ | ~~#7 No current_hp~~ (fixed) |
| ~~Low~~ | ~~#8 Debug print~~ (fixed) |
| Low | #9, #10 |