# Hearthside Secrets - Comprehensive Code Review

## Executive Summary

**Status**: ⚠️ **CRITICAL ISSUES FOUND**

The addon has several breakpoints that will prevent it from working correctly. The main issue is a **mismatch between the external game API and how UI is created**.

---

## 🚨 Critical Issues

### **Issue #1: Missing `ctx.gameContainer` Parameter**

**Severity**: CRITICAL - Game will not display UI

**Location**: `game.lua` lines 38-39, 78-79

**Problem**:
```lua
if HS.UI.HostUI and ctx.gameContainer then
  local ui = HS.UI.HostUI:Create(ctx.gameContainer, gameController)
```

**Why it fails**:
- The external game API context (`ctx`) does NOT include `gameContainer`
- Looking at `Core/Games.lua` lines 126-155, the context only provides:
  - `isHost`
  - `players`
  - `sessionId`
  - `sendMessage()`
  - `print()`
  - `debug()`
- **`gameContainer` is never set in the context**

**Result**: UI will never be created because `ctx.gameContainer` is always `nil`

---

### **Issue #2: UI Creation Pattern Mismatch**

**Severity**: CRITICAL - Wrong UI creation approach

**Problem**:
The external game API expects games to use `createUI` callback (optional), but Hearthside Secrets is trying to create UI inside `onHost`/`onJoin` callbacks.

**Current approach** (WRONG):
```lua
onHost = function(ctx)
  -- Create game controller
  local gameController = HS.Core.GameController.new(true)
  
  -- Try to create UI here (FAILS - no gameContainer)
  if HS.UI.HostUI and ctx.gameContainer then
    local ui = HS.UI.HostUI:Create(ctx.gameContainer, gameController)
  end
end
```

**Expected approach** (from Core/Games.lua):
```lua
-- External game can optionally provide createUI
if def.createUI then
  local uiSuccess, ui = pcall(def.createUI, UIParent, ctx)
  if uiSuccess and ui then
    S.activeGameUI = ui
    if ui.Show then ui:Show() end
  end
end
```

---

### **Issue #3: Update Loop Ticker Management**

**Severity**: MEDIUM - Memory leak potential

**Location**: `game.lua` lines 48-56, 88-96

**Problem**:
```lua
local ticker = C_Timer.NewTicker(0.1, function()
  if gameController then
    gameController:Update(0.1)
    HS.UI.HostUI:Update(gameController)
  end
end)

gameController.ticker = ticker
```

**Issues**:
1. Ticker is stored on `gameController` but never properly cleaned up
2. `onDestroy` tries to cancel `GS.State.activeController.ticker`, not the local ticker
3. If game is destroyed and restarted, old tickers keep running

---

### **Issue #4: GameController Missing Setup Method**

**Severity**: MEDIUM - Initialization incomplete

**Location**: `core/GameController.lua` line 43

**Problem**:
```lua
function GameController:SetupEventHandlers()
  if self.eventFrame then
    return
  end
  -- ... creates event frame
end
```

But in the constructor (line 26-40), there's a call to:
```lua
self:SetupEventHandlers()
```

However, this method was added later and may not exist in the actual file, causing initialization to fail silently.

---

### **Issue #5: Missing Error Handling in StartGame**

**Severity**: MEDIUM - Silent failures

**Location**: `core/GameController.lua` lines 115-171

**Problem**:
```lua
function GameController:StartGame(scenarioId)
  -- Load scenario
  local scenario = HS.Data.Scenarios[scenarioId]
  if not scenario then
    GS.Utils.LogError("Scenario not found: %s", scenarioId)
    return false  -- Returns false but caller doesn't check
  end
```

**Issue**: Callers don't check return value, so failures are silent.

---

### **Issue #6: Player List Not Populated**

**Severity**: HIGH - Role assignment will fail

**Location**: `core/GameController.lua` line 140

**Problem**:
```lua
-- Get player list from participant roster
local playerList = HS.Core.ParticipantRoster:GetAllNames()
if not playerList or #playerList == 0 then
  GS.Utils.LogError("No players in roster")
  return false
end
```

**Issue**: `ParticipantRoster` is never populated with players. The external game API provides `ctx.players` but this is never synced to the ParticipantRoster.

---

### **Issue #7: Network Manager Not Initialized**

**Severity**: HIGH - Multiplayer won't work

**Location**: `game.lua` lines 13-16

**Problem**:
```lua
-- Initialize network
if HS.Net and HS.Net.NetworkManager then
  HS.Net.NetworkManager:Init()
end
```

**Issue**: NetworkManager is initialized globally, but it needs the game controller context to send messages properly. The network manager has no reference to the active game.

---

## 📊 Load Order Analysis

### **Current .toc Order** (Lines 126-142):
```
✅ manifest.lua          - Game metadata (OK)
✅ Init.lua              - Namespace setup (OK)
✅ ParticipantRoster.lua - Player tracking (OK)
✅ PortraitManager.lua   - Portrait display (OK)
✅ GameController.lua    - Main game logic (OK)
✅ ScenarioManager.lua   - Scenario loading (OK)
✅ RoleManager.lua       - Role assignment (OK)
✅ LocationManager.lua   - Location system (OK)
✅ ClueManager.lua       - Clue distribution (OK)
✅ AccusationManager.lua - Accusation handling (OK)
✅ NetworkManager.lua    - Network sync (OK)
✅ SpeakerFrame.lua      - Dialogue UI (OK)
✅ HostUI.lua            - Host interface (OK)
✅ PlayerUI.lua          - Player interface (OK)
✅ missing_brew.lua      - Scenario data (OK)
✅ game.lua              - Registration (OK)
```

**Load order is correct** - dependencies are loaded before dependents.

---

## 🔄 Game Flow Analysis

### **Expected Flow**:
```
1. User clicks game in dropdown
2. GS.Games:RegisterExternal creates wrapper
3. Wrapper calls onHost(ctx) or onJoin(ctx)
4. Game creates controller
5. Game creates UI
6. Game starts update loop
7. User plays game
8. Game ends, onDestroy called
9. Cleanup
```

### **Actual Flow** (BROKEN):
```
1. User clicks game in dropdown ✅
2. GS.Games:RegisterExternal creates wrapper ✅
3. Wrapper calls onHost(ctx) ✅
4. Game creates controller ✅
5. Game tries to create UI ❌ FAILS - ctx.gameContainer is nil
6. Game creates update loop ⚠️ Works but leaks
7. User sees nothing ❌ No UI
8. Game cannot be played ❌
```

---

## 🔧 Required Fixes

### **Fix #1: Remove gameContainer dependency**

**Change in `game.lua`**:
```lua
-- BEFORE (BROKEN)
if HS.UI.HostUI and ctx.gameContainer then
  local ui = HS.UI.HostUI:Create(ctx.gameContainer, gameController)
end

-- AFTER (FIXED)
if HS.UI.HostUI then
  local ui = HS.UI.HostUI:Create(UIParent, gameController)
end
```

### **Fix #2: Proper ticker cleanup**

**Change in `game.lua`**:
```lua
-- Store ticker in State, not on controller
GS.State.activeGameTicker = ticker

-- In onDestroy:
if GS.State.activeGameTicker then
  GS.State.activeGameTicker:Cancel()
  GS.State.activeGameTicker = nil
end
```

### **Fix #3: Populate player roster**

**Change in `game.lua` onHost/onJoin**:
```lua
-- Sync players from context to ParticipantRoster
if ctx.players and HS.Core.ParticipantRoster then
  for _, playerName in ipairs(ctx.players) do
    HS.Core.ParticipantRoster:AddPlayer(playerName)
  end
end
```

### **Fix #4: Error handling**

**Change in `ui/HostUI.lua`**:
```lua
function HostUI:OnStartGame(gameController)
  local success = gameController:StartGame(firstScenario)
  if not success then
    GS.Utils.Print("Failed to start game - check player count")
  end
end
```

---

## 📝 Additional Recommendations

### **1. Add Validation**
- Check that all required systems exist before registration
- Validate context parameters
- Add defensive nil checks

### **2. Improve Logging**
- Add more debug output for flow tracking
- Log all critical failures
- Add success confirmations

### **3. UI Lifecycle**
- Implement proper Show/Hide methods
- Clean up frames on destroy
- Prevent duplicate UI creation

### **4. Network Sync**
- Ensure NetworkManager has game context
- Implement proper message routing
- Add connection state tracking

---

## ✅ What Works

1. **Load order** - Files load in correct sequence
2. **Namespace setup** - GS.HS properly initialized
3. **Registration** - Game registers with RegisterExternal
4. **Core systems** - All managers are properly structured
5. **Data** - Scenarios, roles, clues are well-defined
6. **Developer mode** - 2-player testing works

---

## ❌ What's Broken

1. **UI Creation** - ctx.gameContainer doesn't exist
2. **Player roster** - Not populated from ctx.players
3. **Ticker cleanup** - Memory leak potential
4. **Error handling** - Silent failures
5. **Network context** - Not properly initialized

---

## 🎯 Priority Fixes

### **P0 - Blocking** (Must fix to work at all):
1. Remove ctx.gameContainer dependency
2. Populate player roster from ctx.players

### **P1 - Critical** (Must fix for stability):
3. Fix ticker cleanup
4. Add error handling

### **P2 - Important** (Should fix soon):
5. Network manager context
6. UI lifecycle management

---

## 🧪 Testing Checklist

After fixes, test:
- [ ] Game appears in dropdown
- [ ] Host can start game
- [ ] UI appears for host
- [ ] UI appears for players
- [ ] Roles are assigned
- [ ] Phases advance
- [ ] Timer works
- [ ] Images display
- [ ] Narration shows
- [ ] Game can be destroyed cleanly
- [ ] Can start new game after destroying

---

**Conclusion**: The addon has good architecture but critical implementation issues prevent it from working. The main problem is assuming `ctx.gameContainer` exists when it doesn't. Fix this and the player roster population, and the game should work.
