BreakEscape/planning_notes/player_preferences/CODEBASE_REVIEW.md

Codebase Review - Player Preferences Integration

Date: 2026-02-11
Purpose: Review existing codebase and identify integration points for player preferences system

---

Existing Architecture Summary

Current Player System

Polymorphic Player Pattern ✅

• Already implemented in Game model: belongs_to :player, polymorphic: true
• ApplicationController#current_player handles both modes:
  • Standalone: Uses DemoUser (auto-creates demo_player)
  • Hacktivity: Uses parent app's current_user
• Pundit authorization uses current_player via pundit_user

DemoUser Model Status

Current Implementation:

class DemoUser < ApplicationRecord
  has_many :games, as: :player
  validates :handle, presence: true, uniqueness: true
  
  def admin?
    role == 'admin'
  end
end

Needs Adding:

• has_one :preference association ✅ Planned
• ensure_preference! method ✅ Planned

Game Creation Flow

Current Flow (GamesController#create):

1. Find mission
2. Authorize with Pundit
3. Build player_state with VM/flags context
4. Set player_state BEFORE save (critical for callbacks)
5. Save triggers callbacks:
   • before_create :generate_scenario_data
   • before_create :initialize_player_state
6. Redirect to game_path(@game)

Integration Point: Add sprite validation AFTER step 5, BEFORE step 6

Scenario Generation

Current System:

def generate_scenario_data
  # Build VM context if needed
  vm_context = build_vm_context if mission.requires_vms?
  
  # Generate scenario
  self.scenario_data = mission.generate_scenario_data(vm_context)
end

Integration Point: Add inject_player_preferences call after generation

---

Integration Analysis

1. Routes ✅ Clean Integration

Current Routes:

• /missions - Mission index
• /missions/:id - Mission show
• /games/new - VM set selection
• /games/:id - Game view
• /games POST - Game creation

New Route Needed:

• /configuration GET/PATCH - Player preferences

Potential Conflict: NONE - Configuration route doesn't overlap

---

2. Controllers ✅ Minimal Changes

GamesController Changes Required:

def create
  # ... existing code ...
  @game.save!
  
  # NEW: Check sprite preference
  player_pref = current_player_preference || create_default_preference
  
  if !player_pref.sprite_selected?
    flash[:alert] = 'Please select your character before starting.'
    redirect_to configuration_path(game_id: @game.id) and return
  elsif !player_pref.sprite_valid_for_scenario?(@game.scenario_data)
    flash[:alert] = 'Your selected character is not available for this mission.'
    redirect_to configuration_path(game_id: @game.id) and return
  end
  
  redirect_to game_path(@game)
end

private

def current_player_preference
  if defined?(current_user) && current_user
    current_user.break_escape_preference
  elsif current_demo_user
    current_demo_user.preference
  end
end

Potential Issue: Need access to current_demo_user in addition to current_player

Solution: ApplicationController already has current_player - use that

---

3. Models ✅ Good Fit

Game Model Changes:

def generate_scenario_data
  # ... existing VM context code ...
  
  self.scenario_data = mission.generate_scenario_data(vm_context)
  
  # NEW: Inject player preferences
  inject_player_preferences(self.scenario_data)
end

private

def inject_player_preferences(scenario_data)
  player_pref = player.respond_to?(:break_escape_preference) ? 
                player.break_escape_preference : 
                player.preference
  
  return unless player_pref
  
  scenario_data['player'] ||= {}
  scenario_data['player']['spriteSheet'] = player_pref.selected_sprite
  scenario_data['player']['displayName'] = player_pref.in_game_name
end

Potential Issue: What if player_pref is nil or sprite is nil?

Solution: GamesController validation ensures sprite selected before game creation

---

4. Views ✅ Clean Addition

Current View Structure:

• app/views/break_escape/missions/index.html.erb - Mission selection
• app/views/break_escape/missions/show.html.erb - (doesn't exist, need to check)
• app/views/break_escape/games/new.html.erb - VM set selection
• app/views/break_escape/games/show.html.erb - Game view

New View Needed:

• app/views/break_escape/player_preferences/show.html.erb

No Conflicts: Configuration is a separate page

---

5. Policies ✅ Simple Addition

Existing Pattern:

class ApplicationPolicy
  attr_reader :user, :record
  
  def initialize(user, record)
    @user = user
    @record = record
  end
  
  # ... permission methods ...
end

New Policy:

class PlayerPreferencePolicy < ApplicationPolicy
  def show?
    player_owns_preference?
  end
  
  def update?
    player_owns_preference?
  end
  
  private
  
  def player_owns_preference?
    record.player_type == user.class.name && record.player_id == user.id
  end
end

No Conflicts: Follows existing pattern

---

6. Assets ✅ Well-Structured

Current Asset Structure:

public/break_escape/
├── js/
│   ├── main.js
│   ├── core/
│   ├── systems/
│   ├── minigames/
│   └── ui/
├── css/
│   ├── main.css
│   ├── utilities.css
│   └── (many minigame-specific)
├── assets/
│   ├── characters/ (16 sprite atlases)
│   └── icons/ (including padlock_32.png)

New Files Needed:

public/break_escape/
├── js/
│   └── ui/
│       └── sprite-grid.js ✅ Clean addition
└── css/ (via Rails asset pipeline)
    └── break_escape/
        └── player_preferences.css

Location Note: CSS should go in public/break_escape/css/ (Break Escape convention)

---

7. Phaser Integration ✅ Compatible

Current Phaser Usage:

• Game view loads Phaser 3.60.0 via CDN
• Main game creates single Phaser instance
• Player sprite loaded via window.gameScenario?.player?.spriteSheet

Configuration View:

• Separate page (not in game)
• Can load Phaser independently
• Won't conflict with game instance

Player.js Current Code:

const playerSprite = window.gameScenario?.player?.spriteSheet || 'hacker';
console.log(`Loading player sprite: ${playerSprite}`);

After Integration:

// Now loads from player preferences injected into scenario
const playerSprite = window.gameScenario?.player?.spriteSheet || 'female_hacker_hood';
const playerName = window.gameScenario?.player?.displayName || 'Zero';

---

Identified Issues & Solutions

Issue 1: current_demo_user Not Defined

Problem: current_demo_user method doesn't exist in ApplicationController

Solution: Use current_player directly (already returns correct polymorphic player)

def current_player_preference
  if current_player.respond_to?(:break_escape_preference)
    current_player.break_escape_preference
  elsif current_player.respond_to?(:preference)
    current_player.preference
  end
end

---

Issue 2: Hacktivity User Model Integration

Problem: Parent app's User model needs has_one :break_escape_preference

Solution:

• Document in README for Hacktivity integration
• Not part of Break Escape codebase
• Add migration note in planning docs

---

Issue 3: CSS Location Inconsistency

Problem: Plan shows app/assets/stylesheets/ but game CSS in public/break_escape/css/

Current Pattern: Break Escape uses public/break_escape/css/ for all styles

Solution: Put player_preferences.css in public/break_escape/css/ to match existing pattern

---

Issue 4: Default Sprite Fallback in Player.js

Problem: If sprite is NULL (shouldn't happen), player.js needs fallback

Current Code:

const playerSprite = window.gameScenario?.player?.spriteSheet || 'hacker';

After Integration:

const playerSprite = window.gameScenario?.player?.spriteSheet || 'female_hacker_hood';

Safety: GamesController already validates sprite before game creation, so fallback never triggered

---

Issue 5: Preference Creation Timing

Problem: When should preference record be created?

Options:

1. On first game creation (current plan)
2. On first login/session
3. Via middleware on any request

Recommended: Option 1 (current plan)

• Lazy creation on game creation
• Redirects to configuration if sprite NULL
• No unnecessary records for users who never play

---

Migration Path

Phase 1: Database ✅ No Conflicts

• Create break_escape_player_preferences table
• Unique index on [player_type, player_id]
• No foreign key conflicts

Phase 2: Models ✅ Clean Addition

• Add PlayerPreference model
• Update DemoUser with has_one :preference
• Modify Game#generate_scenario_data to inject preferences

Phase 3: Controllers ✅ Minimal Changes

• Create PlayerPreferencesController
• Modify GamesController#create (add 5 lines for validation)
• No changes to other controllers

Phase 4: Views ✅ New View Only

• Create player_preferences/show.html.erb
• No changes to existing views
• Add CSS file to public/break_escape/css/

Phase 5: JavaScript ✅ New File Only

• Create sprite-grid.js in public/break_escape/js/ui/
• No changes to existing JS
• Player.js already handles dynamic sprite loading

Phase 6: Routes ✅ Simple Addition

• Add 2 routes: get and patch for /configuration
• No conflicts with existing routes

---

Testing Integration Points

1. Current Test Structure

Check existing test files:

test/
├── controllers/
│   └── break_escape/
│       ├── games_controller_test.rb ✅ Will need updates
│       └── missions_controller_test.rb
├── models/
│   └── break_escape/
│       ├── game_test.rb ✅ Will need updates
│       └── demo_user_test.rb ✅ Will need updates
└── integration/

2. New Tests Needed

• test/models/break_escape/player_preference_test.rb (new)
• test/controllers/break_escape/player_preferences_controller_test.rb (new)
• test/policies/break_escape/player_preference_policy_test.rb (new)
• test/integration/sprite_selection_flow_test.rb (new)

3. Existing Tests to Update

• games_controller_test.rb - Add tests for sprite validation flow
• demo_user_test.rb - Test ensure_preference! method
• game_test.rb - Test inject_player_preferences

---

Compatibility Matrix

Component	Current State	Integration	Risk Level	Notes
Polymorphic Player	✅ Exists	✅ Compatible	🟢 None	Already handles both user types
Game Creation	✅ Stable	⚠️ Modification	🟡 Low	Add validation before redirect
Scenario Generation	✅ Stable	⚠️ Modification	🟡 Low	Add injection after generation
Routes	✅ Stable	✅ Addition	🟢 None	New routes don't conflict
Views	✅ Stable	✅ Addition	🟢 None	Separate configuration page
Phaser Integration	✅ Stable	✅ Compatible	🟢 None	Uses existing sprite system
CSS Structure	✅ Stable	✅ Addition	🟢 None	Follow public/ pattern
JavaScript	✅ Stable	✅ Addition	🟢 None	New UI file only
Policies	✅ Stable	✅ Addition	🟢 None	Follows existing pattern
Tests	✅ Stable	⚠️ Expansion	🟡 Low	New tests + minor updates

Overall Risk: 🟢 LOW - Well-isolated feature with clean integration points

---

Recommendations

1. Use Existing Patterns ✅

All planned code follows existing Break Escape conventions:

• Polymorphic associations
• Pundit authorization
• Engine routing
• Public asset structure

2. Update Plan: CSS Location

Change:

- public/break_escape/css/player_preferences.css ✅
+ public/break_escape/css/player_preferences.css

Reason: Match existing asset structure

3. Update Plan: current_player Usage

Change: Use current_player instead of inventing current_demo_user

Reason: Already exists and returns correct polymorphic player

4. Add Safety Check in inject_player_preferences

Add:

def inject_player_preferences(scenario_data)
  player_pref = # ... get preference ...
  
  return unless player_pref&.selected_sprite # Safety: don't inject if nil
  
  scenario_data['player'] ||= {}
  scenario_data['player']['spriteSheet'] = player_pref.selected_sprite
  scenario_data['player']['displayName'] = player_pref.in_game_name
end

---

Conclusion

✅ Ready for Implementation

The planned player preferences system integrates cleanly with the existing codebase:

1. Minimal changes to existing code (< 20 lines modified)
2. No breaking changes to existing functionality
3. Follows established patterns throughout
4. Low risk of conflicts or regressions
5. Well-isolated feature with clear boundaries

Proceed with Implementation

All integration points are clear, and no architectural changes are needed. The plan can be executed as documented with the minor adjustments noted above.

---

Status: ✅ Codebase review complete, ready for Phase 1