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>

src/static/charts.js

@@ -13,6 +13,8 @@ class Charts {
         /* A list of bound charts this is necessary for maintaining a dynamic
            number of charts with their position and zoom factors bound.*/
         this.bound_charts=[];
+        // Debounce timestamp to prevent infinite loop in chart synchronization
+        this._lastSyncTime = 0;
         // Only the main chart is created by default.
         this.create_main_chart();
     }
@@ -26,8 +28,8 @@ class Charts {
         // Display the trading pair as a watermark overlaying the chart.
         this.addWatermark(this.chart_1, this.trading_pair);
 
-        // - Create the candle stick series for our chart
-        this.candleSeries = this.chart_1.addCandlestickSeries();
+        // - Create the candle stick series for our chart (v5 API)
+        this.candleSeries = this.chart_1.addSeries(LightweightCharts.CandlestickSeries);
 
         // Initialize the candlestick series if price_history is available
         if (this.price_history && this.price_history.length > 0) {
@@ -78,9 +80,9 @@ class Charts {
         this.addWatermark(this.chart5, 'Patterns');
         this.bind_charts(this.chart5);
 
-        // Sync time scale with main chart so timestamps align
+        // Sync time scale with main chart so timestamps align (v5 API)
         if (this.chart_1) {
-            let barSpacing = this.chart_1.timeScale().getBarSpacing();
+            let barSpacing = this.chart_1.timeScale().options().barSpacing;
             let scrollPosition = this.chart_1.timeScale().scrollPosition();
             this.chart5.timeScale().applyOptions({ rightOffset: scrollPosition, barSpacing: barSpacing });
         }
@@ -137,16 +139,20 @@ class Charts {
     }
 
 
-    addWatermark(chart,text){
-        chart.applyOptions({
-            watermark: {visible: true,
-                color: '#DBC29E',
-                text: text,
-			    fontSize: 30,
-			    fontFamily: 'Roboto',
-			    fontStyle: 'bold',
-			    vertAlign: 'center'
-            }
+    addWatermark(chart, text){
+        // v5 API: watermarks are now created via createTextWatermark plugin
+        LightweightCharts.createTextWatermark(chart.panes()[0], {
+            horzAlign: 'center',
+            vertAlign: 'center',
+            lines: [
+                {
+                    text: text,
+                    color: '#DBC29E',
+                    fontSize: 30,
+                    fontFamily: 'Roboto',
+                    fontStyle: 'bold',
+                }
+            ]
         });
     }
 
@@ -181,8 +187,11 @@ class Charts {
     bind2charts(){
         //On change in chart 1 change chart 2
         let syncHandler1 = (e) => {
-            // Get the barSpacing(zoom) and position of 1st chart.
-            let barSpacing1 = this.bound_charts[0].timeScale().getBarSpacing();
+            const now = Date.now();
+            if (now - this._lastSyncTime < 50) return;
+            this._lastSyncTime = now;
+            // Get the barSpacing(zoom) and position of 1st chart (v5 API: options().barSpacing)
+            let barSpacing1 = this.bound_charts[0].timeScale().options().barSpacing;
             let scrollPosition1 = this.bound_charts[0].timeScale().scrollPosition();
             // Apply barSpacing(zoom) and position to 2nd chart.
             this.bound_charts[1].timeScale().applyOptions({ rightOffset: scrollPosition1, barSpacing: barSpacing1 });
@@ -191,8 +200,11 @@ class Charts {
 
         //On change in chart 2 change chart 1
         let syncHandler2 = (e) => {
-            // Get the barSpacing(zoom) and position of chart 2
-            let barSpacing2 = this.bound_charts[1].timeScale().getBarSpacing();
+            const now = Date.now();
+            if (now - this._lastSyncTime < 50) return;
+            this._lastSyncTime = now;
+            // Get the barSpacing(zoom) and position of chart 2 (v5 API: options().barSpacing)
+            let barSpacing2 = this.bound_charts[1].timeScale().options().barSpacing;
             let scrollPosition2 = this.bound_charts[1].timeScale().scrollPosition();
             // Apply barSpacing(zoom) and position to chart 1
             this.bound_charts[0].timeScale().applyOptions({ rightOffset: scrollPosition2, barSpacing: barSpacing2 });
@@ -203,8 +215,11 @@ class Charts {
 
         //On change to chart 1 change chart 2 and 3
         let syncHandler = (e) => {
-            // Get the barSpacing(zoom) and position of chart 1
-            let barSpacing1 = this.bound_charts[0].timeScale().getBarSpacing();
+            const now = Date.now();
+            if (now - this._lastSyncTime < 50) return;
+            this._lastSyncTime = now;
+            // Get the barSpacing(zoom) and position of chart 1 (v5 API)
+            let barSpacing1 = this.bound_charts[0].timeScale().options().barSpacing;
             let scrollPosition1 = this.bound_charts[0].timeScale().scrollPosition();
             // Apply barSpacing(zoom) and position to new chart
             this.bound_charts[1].timeScale().applyOptions({ rightOffset: scrollPosition1, barSpacing: barSpacing1 });
@@ -214,8 +229,11 @@ class Charts {
 
         //On change to chart 2 change chart 1 and 3
         let syncHandler2 = (e) => {
-            // Get the barSpacing(zoom) and position of chart 2
-            let barSpacing2 = this.bound_charts[1].timeScale().getBarSpacing();
+            const now = Date.now();
+            if (now - this._lastSyncTime < 50) return;
+            this._lastSyncTime = now;
+            // Get the barSpacing(zoom) and position of chart 2 (v5 API)
+            let barSpacing2 = this.bound_charts[1].timeScale().options().barSpacing;
             let scrollPosition2 = this.bound_charts[1].timeScale().scrollPosition();
             // Apply barSpacing(zoom) and position to chart 1 and 3
             this.bound_charts[0].timeScale().applyOptions({ rightOffset: scrollPosition2, barSpacing: barSpacing2 });
@@ -225,8 +243,11 @@ class Charts {
 
         //On change to chart 3 change chart 1 and 2
         let syncHandler3 = (e) => {
-            // Get the barSpacing(zoom) and position of new chart
-            let barSpacing2 = this.bound_charts[2].timeScale().getBarSpacing();
+            const now = Date.now();
+            if (now - this._lastSyncTime < 50) return;
+            this._lastSyncTime = now;
+            // Get the barSpacing(zoom) and position of new chart (v5 API)
+            let barSpacing2 = this.bound_charts[2].timeScale().options().barSpacing;
             let scrollPosition2 = this.bound_charts[2].timeScale().scrollPosition();
             // Apply barSpacing(zoom) and position to parent chart
             this.bound_charts[0].timeScale().applyOptions({ rightOffset: scrollPosition2, barSpacing: barSpacing2 });
@@ -236,10 +257,13 @@ class Charts {
     }
 
     bind4charts(){
-        // Sync all 4 charts together
+        // Sync all 4 charts together (v5 API: options().barSpacing)
         let syncFromChart = (sourceIndex) => {
             return (e) => {
-                let barSpacing = this.bound_charts[sourceIndex].timeScale().getBarSpacing();
+                const now = Date.now();
+                if (now - this._lastSyncTime < 50) return;
+                this._lastSyncTime = now;
+                let barSpacing = this.bound_charts[sourceIndex].timeScale().options().barSpacing;
                 let scrollPosition = this.bound_charts[sourceIndex].timeScale().scrollPosition();
                 for (let i = 0; i < 4; i++) {
                     if (i !== sourceIndex) {
@@ -255,10 +279,13 @@ class Charts {
     }
 
     bind5charts(){
-        // Sync all 5 charts together (main + RSI + MACD + %B + Patterns)
+        // Sync all 5 charts together (main + RSI + MACD + %B + Patterns) (v5 API)
         let syncFromChart = (sourceIndex) => {
             return (e) => {
-                let barSpacing = this.bound_charts[sourceIndex].timeScale().getBarSpacing();
+                const now = Date.now();
+                if (now - this._lastSyncTime < 50) return;
+                this._lastSyncTime = now;
+                let barSpacing = this.bound_charts[sourceIndex].timeScale().options().barSpacing;
                 let scrollPosition = this.bound_charts[sourceIndex].timeScale().scrollPosition();
                 for (let i = 0; i < 5; i++) {
                     if (i !== sourceIndex) {
@@ -339,14 +366,20 @@ class Charts {
 
         console.log(`Setting ${markers.length} trade markers on chart`);
 
-        // Set markers on the candlestick series
-        this.candleSeries.setMarkers(markers);
+        // v5 API: use createSeriesMarkers instead of series.setMarkers
+        // Clear existing markers primitive if it exists
+        if (this.markersPrimitive) {
+            this.markersPrimitive.setMarkers([]);
+        }
+        // Create new markers primitive
+        this.markersPrimitive = LightweightCharts.createSeriesMarkers(this.candleSeries, markers);
     }
 
     // Clear all trade markers from chart
     clearTradeMarkers() {
-        if (this.candleSeries) {
-            this.candleSeries.setMarkers([]);
+        // v5 API: clear markers via the markers primitive
+        if (this.markersPrimitive) {
+            this.markersPrimitive.setMarkers([]);
             console.log('Trade markers cleared');
         }
     }

src/static/general.js

@@ -15,7 +15,7 @@ class User_Interface {
         this.account = new Account();
 
         // Register a callback function for when indicator updates are received from the data object
-        this.data.registerCallback_i_updates(this.indicators.update);
+        this.data.registerCallback_i_updates(this.indicators.update.bind(this.indicators));
 
         // Initialize all components after the page has loaded and Blockly is ready
         this.initializeAll();

src/static/indicators.js

@@ -14,8 +14,12 @@ class Indicator_Output {
         this.legend[name].style.left = 3 + 'px';
         this.legend[name].style.top = 3 + 'px';
         // subscribe set legend text to crosshair moves
+        // v5 API: seriesPrices renamed to seriesData, returns full data item
         chart.subscribeCrosshairMove((param) => {
-            this.set_legend_text(param.seriesPrices.get(lineSeries), name);
+            const data = param.seriesData.get(lineSeries);
+            // Extract value from data item (could be {value} or {close} etc)
+            const priceValue = data ? (data.value !== undefined ? data.value : data.close) : undefined;
+            this.set_legend_text(priceValue, name);
         });
     }
 
@@ -88,21 +92,27 @@ class Indicator {
     }
 
     addHist(name, chart, color = '#26a69a') {
-        this.hist[name] = chart.addHistogramSeries({
+        // v5 API: use addSeries with HistogramSeries
+        this.hist[name] = chart.addSeries(LightweightCharts.HistogramSeries, {
             color: color,
             priceFormat: {
                 type: 'price',
             },
             priceScaleId: 'volume_ps',
+        });
+        // v5: scaleMargins must be set on the price scale, not series options
+        // Volume should only take up bottom 30% of the chart
+        chart.priceScale('volume_ps').applyOptions({
             scaleMargins: {
-                top: 0,
+                top: 0.7,
                 bottom: 0,
             },
         });
     }
 
     addLine(name, chart, color, lineWidth) {
-        this.lines[name] = chart.addLineSeries({
+        // v5 API: use addSeries with LineSeries
+        this.lines[name] = chart.addSeries(LightweightCharts.LineSeries, {
             color: color,
             lineWidth: lineWidth
         });
@@ -111,8 +121,6 @@ class Indicator {
     }
 
     setLine(lineName, data, value_name) {
-        console.log('indicators[68]: setLine takes:(lineName, data, value_name)');
-        console.log(lineName, data, value_name);
 
         let priceValue;
 
@@ -143,6 +151,7 @@ class Indicator {
     }
 
     updateDisplay(name, priceValue, value_name) {
+        // Try the old element format first (legacy chart-based display)
         let element = document.getElementById(this.name + '_' + value_name);
         if (element) {
             if (typeof priceValue === 'object' && priceValue !== null) {
@@ -172,7 +181,36 @@ class Indicator {
             element.style.height = 'auto';  // Reset height
             element.style.height = (element.scrollHeight) + 'px';  // Adjust height based on content
         } else {
-            console.warn(`Element with ID ${this.name}_${value_name} not found.`);
+            // Try the new card-based display element
+            const cardElement = document.getElementById(`indicator_card_value_${this.name}`);
+            if (cardElement) {
+                let displayValue = '--';
+                if (typeof priceValue === 'object' && priceValue !== null) {
+                    // For object values, get the first numeric value
+                    const values = Object.values(priceValue).filter(v => typeof v === 'number' && !isNaN(v));
+                    if (values.length > 0) {
+                        displayValue = this._formatDisplayValue(values[0]);
+                    }
+                } else if (typeof priceValue === 'number' && !isNaN(priceValue)) {
+                    displayValue = this._formatDisplayValue(priceValue);
+                }
+                cardElement.textContent = displayValue;
+            }
+            // Silently ignore if neither element exists (may be during initialization)
+        }
+    }
+
+    /**
+     * Formats a numeric value for display in cards
+     */
+    _formatDisplayValue(value) {
+        if (value === null || value === undefined || isNaN(value)) return '--';
+        if (Math.abs(value) >= 1000) {
+            return value.toFixed(0);
+        } else if (Math.abs(value) >= 100) {
+            return value.toFixed(1);
+        } else {
+            return value.toFixed(2);
         }
     }
 
@@ -181,8 +219,6 @@ class Indicator {
     }
 
     updateLine(name, data, value_name) {
-        console.log('indicators[68]: updateLine takes:(name, data, value_name)');
-        console.log(name, data, value_name);
 
         // Check if the data is a multi-value object
         if (typeof data === 'object' && data !== null && value_name in data) {
@@ -263,7 +299,6 @@ class SMA extends Indicator {
 
     init(data) {
         this.setLine('line', data, 'value');
-        console.log('line data', data);
     }
 
     update(data) {
@@ -574,8 +609,8 @@ class CandlestickPattern extends Indicator {
     }
 
     addPatternHist(name, chart) {
-        // Create histogram series for pattern signals
-        this.hist[name] = chart.addHistogramSeries({
+        // v5 API: use addSeries with HistogramSeries
+        this.hist[name] = chart.addSeries(LightweightCharts.HistogramSeries, {
             priceFormat: {
                 type: 'price',
             },