Address second round of Codex review feedback on formations plan

Fixes:
1. Class signature consistency - reference __init__ from 1.1, don't redefine
2. Dynamic chart ID - use this.data.chart1_id instead of hardcoded 'chart1'
3. Comms wiring - use this.comms.on() pattern like signals.js
4. RAF loop guards - add pause conditions, destroy() teardown hook
5. Drag null guards - validate chartCoords before mutating formation data
6. Test organization - split MVP tests from Phase C target tests

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

FORMATIONS_PLAN.md

@@ -146,8 +146,8 @@ class Formations:
 
 ```python
 class Formations:
-    def __init__(self, database: Database):
-        self.database = database
+    # Full signature - see 1.1 for __init__ implementation
+    # def __init__(self, data_cache: DataCache, database: Database)
 
     # CRUD operations
     def create(self, user_id: int, data: dict) -> dict
@@ -231,17 +231,16 @@ elif message_type == 'delete_formation':
     return {'reply': 'formation_deleted', 'data': result}
 ```
 
-**Frontend handler** (in `communication.js`, follows existing pattern at line ~88):
+**Frontend handler** (in `formations.js`, follows signals.js pattern):
 ```javascript
-// Socket response handler
-socket.on('message', (msg) => {
-    if (msg.reply === 'formations') {
-        UI.formations.handleFormations(msg.data);
-    } else if (msg.reply === 'formation_created') {
-        UI.formations.handleFormationCreated(msg.data);
+// In Formations.registerSocketHandlers() - use comms.on() pattern
+registerSocketHandlers() {
+    this.comms.on('formations', this.handleFormationsResponse.bind(this));
+    this.comms.on('formation_created', this.handleFormationCreated.bind(this));
+    this.comms.on('formation_updated', this.handleFormationUpdated.bind(this));
+    this.comms.on('formation_deleted', this.handleFormationDeleted.bind(this));
+    this.comms.on('formation_error', this.handleFormationError.bind(this));
 }
-    // ... etc
-});
 ```
 
 ---
@@ -310,8 +309,8 @@ Follow the three-class pattern from `signals.js`:
 **Modify:** `src/static/general.js`
 ```javascript
 this.formations = new Formations(this);
-// After charts init - pass candleSeries for v5 coordinate conversion:
-this.formations.initOverlay('chart1', this.charts.chart_1, this.charts.candleSeries);
+// After charts init - use dynamic chart ID, pass candleSeries for v5 coordinate conversion:
+this.formations.initOverlay(this.data.chart1_id, this.charts.chart_1, this.charts.candleSeries);
 ```
 
 ---
@@ -368,16 +367,31 @@ class FormationOverlay {
     _startSyncLoop() {
         // CRITICAL: Use requestAnimationFrame polling, NOT subscribeVisibleTimeRangeChange
         // This avoids the infinite loop problem with bound charts
+        this._loopRunning = true;
         const sync = () => {
+            // Guard: only run if there's something to sync
+            if (!this._loopRunning) return;
+            if (this.formations.size > 0 || this.tempFormation) {
                 this._updateAllPositions();
+            }
             this._animationFrameId = requestAnimationFrame(sync);
         };
         this._animationFrameId = requestAnimationFrame(sync);
     }
 
     stopSyncLoop() {
+        this._loopRunning = false;
         if (this._animationFrameId) {
             cancelAnimationFrame(this._animationFrameId);
+            this._animationFrameId = null;
+        }
+    }
+
+    // Call when component is destroyed
+    destroy() {
+        this.stopSyncLoop();
+        if (this.svg && this.svg.parentNode) {
+            this.svg.parentNode.removeChild(this.svg);
         }
     }
 
@@ -648,13 +662,20 @@ _setupDragListeners() {
         const x = e.clientX - rect.left;
         const y = e.clientY - rect.top;
 
+        // Guard: skip if outside SVG bounds
+        if (x < 0 || y < 0 || x > rect.width || y > rect.height) return;
+
         // Update anchor position
         const { anchor, lineIndex, pointIndex, formation } = this.draggingAnchor;
         anchor.setAttribute('cx', x);
         anchor.setAttribute('cy', y);
 
-        // Update formation data
+        // Update formation data with null guard
         const chartCoords = this._pixelToChart(x, y);
+        if (!chartCoords || chartCoords.time === null || chartCoords.price === null) {
+            return;  // Invalid coordinates - don't mutate formation data
+        }
+
         const pointKey = pointIndex === 0 ? 'point1' : 'point2';
         formation.data.lines[lineIndex][pointKey] = {
             time: chartCoords.time,
@@ -1029,6 +1050,8 @@ Socket handling stays in `BrighterTrades.process_incoming_message()` (not app.py
 
 ## Tests Required
 
+### MVP Tests (Phase A - ship with Line + Channel)
+
 **Backend (`tests/test_formations.py`):**
 ```python
 def test_create_formation_unique_constraint():
@@ -1043,11 +1066,11 @@ def test_calculate_line_value_extrapolation():
 def test_get_by_tbl_key_for_strategy_owner():
     """Strategy uses owner's formations, not current user's"""
 
-def test_calculate_targets_head_shoulders():
-    """Target = neckline - pattern_height"""
+def test_delete_formation():
+    """Formation deleted from DB and cache"""
 
-def test_calculate_targets_double_bottom():
-    """Target = neckline + pattern_height"""
+def test_update_formation_lines():
+    """Lines JSON updated correctly"""
 ```
 
 **Generator (`tests/test_strategy_generation.py`):**
@@ -1056,6 +1079,20 @@ def test_formation_block_generates_process_formation():
     """Block with tbl_key generates correct function call"""
 ```
 
+### Phase C Tests (add when implementing targets)
+
+**Backend (`tests/test_formations.py`):**
+```python
+def test_calculate_targets_head_shoulders():
+    """Target = neckline - pattern_height"""
+
+def test_calculate_targets_double_bottom():
+    """Target = neckline + pattern_height"""
+
+def test_calculate_targets_triangle():
+    """Target = apex projection"""
+```
+
 **Integration (`tests/test_strategy_execution.py`):**
 ```python
 def test_process_formation_in_paper_strategy():