Add script.js cleanup implementation plan

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2026-02-26 08:58:30 +00:00 · 2026-02-26 08:58:30 +00:00 · 3d0a0b33a3
commit 3d0a0b33a3
parent 87bf7b7189
1 changed files with 607 additions and 0 deletions
--- a/docs/plans/2026-02-26-script-js-cleanup-plan.md
+++ b/docs/plans/2026-02-26-script-js-cleanup-plan.md
@ -0,0 +1,607 @@
+# script.js Code Review Cleanup - Implementation Plan
+
+> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task.
+
+**Goal:** Fix memory leaks, reduce console noise, eliminate code duplication, and improve rendering performance in the main frontend script.
+
+**Architecture:** Targeted fixes to a single 2,310-line vanilla JS file. No module splitting, no architectural changes. Each task modifies `static/script.js` and can be tested by refreshing the browser.
+
+**Tech Stack:** Vanilla JavaScript, served directly by FastAPI StaticFiles. No build system.
+
+**CRITICAL:** Do NOT push to git until all tasks pass manual testing.
+
+---
+
+### Task 1: Add DEBUG flag and debugLog helper
+
+**Files:**
+- Modify: `static/script.js:1-3` (top of file)
+
+**Step 1: Add DEBUG flag and helper after first line**
+
+At the top of the file, after `console.log('...')` on line 1, add:
+
+```javascript
+const DEBUG = false;
+function debugLog(...args) { if (DEBUG) console.log(...args); }
+```
+
+**Step 2: Replace all console.log calls with debugLog**
+
+Replace every `console.log(` with `debugLog(` throughout the file. There are ~35 instances.
+
+Keep `console.error(` unchanged - those should always be visible.
+
+Lines to change (all `console.log` → `debugLog`):
+- Lines 100, 104, 106, 109 (chat button clicks)
+- Lines 117, 121, 123, 126 (stats button clicks)
+- Lines 134, 138, 140, 143 (inventory button clicks)
+- Line 563 (heatmap loaded)
+- Line 648 (portals loaded)
+- Line 710 (portals rendered)
+- Lines 729, 732, 739, 742, 786, 789 (stats window)
+- Lines 824, 827, 834, 837, 1029, 1032 (inventory window)
+- Line 1433 (server status)
+- Lines 1511, 1515, 1733 (render cycle)
+- Lines 1801, 1804, 1811, 1814, 1851, 1854 (chat window)
+- Line 2173 (milestone)
+
+**Step 3: Verify**
+
+Open browser, load map page. Console should be clean (no log spam). Temporarily set `DEBUG = true`, refresh - logs should reappear.
+
+**Step 4: Commit**
+
+```bash
+git add static/script.js
+git commit -m "Add DEBUG flag and gate console.log behind debugLog helper"
+```
+
+---
+
+### Task 2: Add named constants for magic numbers
+
+**Files:**
+- Modify: `static/script.js:298-300` (constants block)
+- Modify: `static/script.js:556` (heatmap fetch)
+- Modify: `static/script.js:1448-1452` (polling intervals)
+- Modify: `static/script.js:2069` (notification duration)
+- Modify: `static/script.js:2148` (glow duration)
+
+**Step 1: Add constants after existing POLL_MS definition (line 300)**
+
+```javascript
+const POLL_RARES_MS = 300000;           // 5 minutes
+const POLL_KILLS_MS = 300000;           // 5 minutes
+const POLL_HEALTH_MS = 30000;           // 30 seconds
+const NOTIFICATION_DURATION_MS = 6000;  // Rare notification display time
+const GLOW_DURATION_MS = 5000;          // Player glow after rare find
+const MAX_HEATMAP_POINTS = 50000;
+const HEATMAP_HOURS = 24;
+```
+
+**Step 2: Replace hardcoded values**
+
+Line 556 - heatmap fetch URL:
+```javascript
+// Before:
+const response = await fetch(`${API_BASE}/spawns/heatmap?hours=24&limit=50000`);
+// After:
+const response = await fetch(`${API_BASE}/spawns/heatmap?hours=${HEATMAP_HOURS}&limit=${MAX_HEATMAP_POINTS}`);
+```
+
+Lines 1448-1452 - polling intervals:
+```javascript
+// Before:
+setInterval(pollTotalRares, 300000);
+setInterval(pollTotalKills, 300000);
+setInterval(pollServerHealth, 30000);
+// After:
+setInterval(pollTotalRares, POLL_RARES_MS);
+setInterval(pollTotalKills, POLL_KILLS_MS);
+setInterval(pollServerHealth, POLL_HEALTH_MS);
+```
+
+Line 2069 - notification timeout:
+```javascript
+// Before:
+}, 6000);
+// After:
+}, NOTIFICATION_DURATION_MS);
+```
+
+Line 2148 - glow timeout:
+```javascript
+// Before:
+}, 5000);
+// After:
+}, GLOW_DURATION_MS);
+```
+
+**Step 3: Verify**
+
+Refresh page. Players load, positions update every 2s. Heatmap toggle works. (These confirm intervals and heatmap constants are correct.)
+
+**Step 4: Commit**
+
+```bash
+git add static/script.js
+git commit -m "Replace magic numbers with named constants"
+```
+
+---
+
+### Task 3: Fix polling interval memory leak
+
+**Files:**
+- Modify: `static/script.js:1440-1453` (startPolling function)
+
+**Step 1: Replace startPolling function**
+
+Replace the current `startPolling()` function (lines 1440-1453) with:
+
+```javascript
+const pollIntervals = [];
+
+function startPolling() {
+    // Clear any existing intervals first (prevents leak on re-init)
+    pollIntervals.forEach(id => clearInterval(id));
+    pollIntervals.length = 0;
+
+    // Initial fetches
+    pollLive();
+    pollTotalRares();
+    pollTotalKills();
+    pollServerHealth();
+
+    // Set up recurring polls
+    pollIntervals.push(setInterval(pollLive, POLL_MS));
+    pollIntervals.push(setInterval(pollTotalRares, POLL_RARES_MS));
+    pollIntervals.push(setInterval(pollTotalKills, POLL_KILLS_MS));
+    pollIntervals.push(setInterval(pollServerHealth, POLL_HEALTH_MS));
+}
+```
+
+Also remove the old `pollID` variable and the `if (pollID !== null) return;` guard since the new function handles re-initialization properly.
+
+**Step 2: Verify**
+
+Refresh page. Players load, update every 2s. Open DevTools > Performance Monitor - verify no interval accumulation. Refresh several times rapidly - should not see multiplied polling.
+
+**Step 3: Commit**
+
+```bash
+git add static/script.js
+git commit -m "Fix polling interval memory leak - store all interval IDs"
+```
+
+---
+
+### Task 4: Fix highlightRareFinder DOM query
+
+**Files:**
+- Modify: `static/script.js:2138-2151` (highlightRareFinder function)
+
+**Step 1: Replace highlightRareFinder function**
+
+Replace lines 2138-2151 with:
+
+```javascript
+function highlightRareFinder(characterName) {
+    // Use element pool for O(1) lookup instead of querySelectorAll
+    for (const item of elementPools.activeListItems) {
+        if (item.playerData && item.playerData.character_name === characterName) {
+            item.classList.add('rare-finder-glow');
+            setTimeout(() => {
+                item.classList.remove('rare-finder-glow');
+            }, GLOW_DURATION_MS);
+            break;
+        }
+    }
+}
+```
+
+**Step 2: Verify**
+
+This fires on rare notifications. If not easily testable live, verify by temporarily adding to console:
+```javascript
+highlightRareFinder('SomeOnlinePlayerName');
+```
+Confirm the player glows briefly in the list.
+
+**Step 3: Commit**
+
+```bash
+git add static/script.js
+git commit -m "Fix highlightRareFinder to use element pool instead of DOM query"
+```
+
+---
+
+### Task 5: Extract createWindow helper
+
+**Files:**
+- Modify: `static/script.js` - add helper before showStatsWindow (~line 725)
+- Modify: `static/script.js:728-794` (showStatsWindow)
+- Modify: `static/script.js:823-1036` (showInventoryWindow)
+- Modify: `static/script.js:1800-1858` (showChatWindow)
+
+**Step 1: Add createWindow helper function**
+
+Insert before `showStatsWindow` (around line 725):
+
+```javascript
+/**
+ * Create or show a draggable window. Returns { win, content, isNew }.
+ * If window already exists, brings it to front and returns isNew: false.
+ */
+function createWindow(id, title, className, options = {}) {
+    const { width = '400px', height = '300px', onClose } = options;
+
+    // Check if window already exists - bring to front
+    const existing = document.getElementById(id);
+    if (existing) {
+        existing.style.display = 'flex';
+        if (!window.__chatZ) window.__chatZ = 10000;
+        window.__chatZ += 1;
+        existing.style.zIndex = window.__chatZ;
+        return { win: existing, content: existing.querySelector('.window-content'), isNew: false };
+    }
+
+    // Create new window
+    if (!window.__chatZ) window.__chatZ = 10000;
+    window.__chatZ += 1;
+
+    const win = document.createElement('div');
+    win.id = id;
+    win.className = className;
+    win.style.display = 'flex';
+    win.style.zIndex = window.__chatZ;
+    win.style.width = width;
+    win.style.height = height;
+
+    const header = document.createElement('div');
+    header.className = 'window-header';
+
+    const titleSpan = document.createElement('span');
+    titleSpan.textContent = title;
+    header.appendChild(titleSpan);
+
+    const closeBtn = document.createElement('button');
+    closeBtn.textContent = '\u00D7';
+    closeBtn.addEventListener('click', () => {
+        win.style.display = 'none';
+        if (onClose) onClose();
+    });
+    header.appendChild(closeBtn);
+
+    const content = document.createElement('div');
+    content.className = 'window-content';
+
+    win.appendChild(header);
+    win.appendChild(content);
+    document.body.appendChild(win);
+    makeDraggable(win, header);
+
+    return { win, content, isNew: true };
+}
+```
+
+**Step 2: Refactor showStatsWindow to use createWindow**
+
+Replace the window creation boilerplate in `showStatsWindow` with:
+
+```javascript
+function showStatsWindow(name) {
+    debugLog('showStatsWindow called for:', name);
+    const windowId = `statsWindow-${name}`;
+
+    const { content, isNew } = createWindow(
+        windowId, `Stats: ${name}`, 'stats-window',
+        { width: '800px', height: '600px' }
+    );
+
+    if (!isNew) {
+        debugLog('Existing stats window found, showing it');
+        return;
+    }
+
+    // Stats-specific content (time range buttons + iframes)
+    // ... keep existing content creation logic from original function ...
+}
+```
+
+Keep ALL the stats-specific logic (time range buttons, iframe creation, updateStatsTimeRange) - only remove the duplicated window setup code.
+
+**Step 3: Refactor showChatWindow to use createWindow**
+
+Same pattern - replace window boilerplate, keep chat-specific content (message list, input field, send button).
+
+**Step 4: Refactor showInventoryWindow to use createWindow**
+
+Same pattern - replace window boilerplate, keep inventory-specific content (grid, icon rendering, tooltips).
+
+**Step 5: Verify**
+
+Test each window type:
+1. Click a player's chat button - window opens, is draggable, close works
+2. Click a player's stats button - window opens, iframes load, time range works
+3. Click a player's inventory button - window opens, items display, tooltips work
+4. Open multiple windows - z-index stacking works (latest on top)
+5. Close and reopen each - brings to front correctly
+
+**Step 6: Commit**
+
+```bash
+git add static/script.js
+git commit -m "Extract createWindow helper to eliminate window creation duplication"
+```
+
+---
+
+### Task 6: Add requestAnimationFrame batching for pan/zoom
+
+**Files:**
+- Modify: `static/script.js` - add scheduleViewUpdate near updateView (~line 1295)
+- Modify: `static/script.js:1908` (wheel handler)
+- Modify: `static/script.js:1919` (mousemove drag handler)
+- Modify: `static/script.js:1935` (touchmove handler)
+
+**Step 1: Add scheduleViewUpdate after updateView function**
+
+After the `updateView()` function (line 1295), add:
+
+```javascript
+let pendingFrame = null;
+function scheduleViewUpdate() {
+    if (!pendingFrame) {
+        pendingFrame = requestAnimationFrame(() => {
+            updateView();
+            pendingFrame = null;
+        });
+    }
+}
+```
+
+**Step 2: Replace updateView() calls in interaction handlers**
+
+Line 1908 (wheel zoom):
+```javascript
+// Before: updateView();
+// After:  scheduleViewUpdate();
+```
+
+Line 1919 (mousemove drag):
+```javascript
+// Before: updateView();
+// After:  scheduleViewUpdate();
+```
+
+Line 1935 (touchmove):
+```javascript
+// Before: updateView();
+// After:  scheduleViewUpdate();
+```
+
+**Keep** the direct `updateView()` calls at:
+- Line 1301 (fitToWindow) - one-time call, not a hot path
+- Line 1767 (selectPlayer) - one-time call on player click
+
+**Step 3: Verify**
+
+Pan the map by dragging - should feel smooth, no jitter. Zoom with scroll wheel - smooth. Toggle heatmap on, zoom/pan - heatmap updates correctly. Overall responsiveness should be same or better.
+
+**Step 4: Commit**
+
+```bash
+git add static/script.js
+git commit -m "Add requestAnimationFrame batching for pan/zoom updates"
+```
+
+---
+
+### Task 7: Optimize renderTrails
+
+**Files:**
+- Modify: `static/script.js:1737-1757` (renderTrails function)
+
+**Step 1: Replace renderTrails function**
+
+Replace lines 1737-1757 with:
+
+```javascript
+function renderTrails(trailData) {
+    trailsContainer.innerHTML = '';
+    // Build point strings directly - avoid intermediate arrays
+    const byChar = {};
+    for (const pt of trailData) {
+        const { x, y } = worldToPx(pt.ew, pt.ns);
+        const key = pt.character_name;
+        if (!byChar[key]) byChar[key] = { points: `${x},${y}`, count: 1 };
+        else { byChar[key].points += ` ${x},${y}`; byChar[key].count++; }
+    }
+    for (const name in byChar) {
+        if (byChar[name].count < 2) continue;
+        const poly = document.createElementNS('http://www.w3.org/2000/svg', 'polyline');
+        poly.setAttribute('points', byChar[name].points);
+        poly.setAttribute('stroke', getColorFor(name));
+        poly.setAttribute('fill', 'none');
+        poly.setAttribute('class', 'trail-path');
+        trailsContainer.appendChild(poly);
+    }
+}
+```
+
+**Step 2: Verify**
+
+Refresh page. Player trails should render as colored lines following player positions. Verify trails match player colors. Multiple players should each have distinct trail lines.
+
+**Step 3: Commit**
+
+```bash
+git add static/script.js
+git commit -m "Optimize renderTrails to build SVG point strings directly"
+```
+
+---
+
+### Task 8: Remove redundant .slice() in renderList
+
+**Files:**
+- Modify: `static/script.js:1487` (renderList function)
+
+**Step 1: Remove .slice()**
+
+Line 1487:
+```javascript
+// Before:
+const sorted = filtered.slice().sort(currentSort.comparator);
+// After:
+filtered.sort(currentSort.comparator);
+const sorted = filtered;
+```
+
+**Step 2: Verify**
+
+Refresh page. Player list should sort correctly. Click each sort button (name, KPH, kills, rares, total kills, KPR) - all should work correctly.
+
+**Step 3: Commit**
+
+```bash
+git add static/script.js
+git commit -m "Remove redundant .slice() before .sort() in renderList"
+```
+
+---
+
+### Task 9: Add centralized error handling
+
+**Files:**
+- Modify: `static/script.js` - add handleError near top (~after debugLog)
+- Modify: `static/style.css` - add .error-toast CSS
+- Modify: `static/script.js` - update catch blocks throughout
+
+**Step 1: Add handleError function after debugLog**
+
+```javascript
+function handleError(context, error, showUI = false) {
+    console.error(`[${context}]`, error);
+    if (showUI) {
+        const msg = document.createElement('div');
+        msg.className = 'error-toast';
+        msg.textContent = `${context}: ${error.message || 'Unknown error'}`;
+        document.body.appendChild(msg);
+        setTimeout(() => msg.remove(), GLOW_DURATION_MS);
+    }
+}
+```
+
+**Step 2: Add CSS for error toast**
+
+In `static/style.css`, add at the end:
+
+```css
+/* Error Toast */
+.error-toast {
+    position: fixed;
+    bottom: 20px;
+    right: 20px;
+    background: rgba(220, 38, 38, 0.9);
+    color: white;
+    padding: 12px 20px;
+    border-radius: 8px;
+    font-size: 13px;
+    z-index: 99999;
+    animation: toastFadeIn 0.3s ease;
+    max-width: 400px;
+}
+
+@keyframes toastFadeIn {
+    from { opacity: 0; transform: translateY(10px); }
+    to { opacity: 1; transform: translateY(0); }
+}
+```
+
+**Step 3: Update catch blocks**
+
+Background polling (showUI = false):
+```javascript
+// pollLive catch (~line 1329):
+// Before: console.error('Live or trails fetch failed:', e);
+// After:  handleError('Player update', e);
+
+// pollTotalRares catch (~line 1339):
+// Before: console.error('Total rares fetch failed:', e);
+// After:  handleError('Rare counter', e);
+
+// pollTotalKills catch (~line 1358):
+// Before: console.error('Total kills fetch failed:', e);
+// After:  handleError('Kill counter', e);
+
+// pollServerHealth catch (~line 1375):
+// Before: console.error('Server health fetch failed:', e);
+// After:  handleError('Server health', e);
+```
+
+User-triggered actions (showUI = true):
+```javascript
+// showInventoryWindow catch (~line 1024):
+// Before: content.innerHTML = `<div class="error">Failed to load...</div>`;
+// After:  handleError('Inventory', err, true);
+//         content.innerHTML = `<div class="error">Failed to load inventory</div>`;
+
+// fetchHeatmapData catch (~line 565):
+// Before: console.error('Heatmap fetch error:', err);
+// After:  handleError('Heatmap', err);
+
+// fetchPortalData catch (~line 650):
+// Before: (similar)
+// After:  handleError('Portals', err);
+```
+
+**Step 4: Verify**
+
+1. Normal operation - no toasts appear
+2. Temporarily break a URL (e.g., change API_BASE) - verify console.error appears with `[context]` prefix
+3. For user-triggered errors (inventory of offline player) - verify red toast appears bottom-right and auto-dismisses
+
+**Step 5: Commit**
+
+```bash
+git add static/script.js static/style.css
+git commit -m "Add centralized error handling with UI toast for user-facing errors"
+```
+
+---
+
+### Task 10: Full Manual Testing Pass
+
+**Files:** None modified - testing only
+
+**CRITICAL: Do not push to git until ALL tests pass.**
+
+**Test Checklist:**
+
+| # | Test | How to Verify | Status |
+|---|------|---------------|--------|
+| 1 | Page load | Map renders, players appear, no console errors | |
+| 2 | Live tracking | Players update positions every ~2 seconds | |
+| 3 | Player filter | Type name in filter, list updates correctly | |
+| 4 | Sort buttons | Each of 6 sort options works (name, KPH, kills, rares, total_kills, KPR) | |
+| 5 | Player selection | Click player in list, map pans and zooms to them | |
+| 6 | Pan/zoom | Mouse drag smooth, scroll wheel zoom smooth, no jitter or lag | |
+| 7 | Heatmap | Toggle on - renders points. Pan/zoom - updates. Toggle off - clears | |
+| 8 | Portals | Toggle on - portal icons appear. Toggle off - clears | |
+| 9 | Chat window | Opens, draggable, close button works, receives WebSocket messages | |
+| 10 | Stats window | Opens, iframes load, time range buttons work, draggable | |
+| 11 | Inventory window | Opens, items display with icons, hover tooltip works, draggable | |
+| 12 | Multiple windows | Open several windows - latest always on top (z-index) | |
+| 13 | Console (DEBUG=false) | Console is clean, only errors/warnings visible | |
+| 14 | Console (DEBUG=true) | Set DEBUG=true in code, refresh - verbose logs appear | |
+| 15 | Page reload x5 | Reload 5 times rapidly - no interval accumulation, consistent behavior | |
+| 16 | Trail rendering | Player trails visible as colored lines matching player dot colors | |
+| 17 | Error handling | Briefly disconnect network - no crashes, console shows `[context]` errors | |
+
+**If all pass:** Ready to push. Inform user and await approval.
+
+**If any fail:** Fix the specific issue, re-test that item, then re-run full checklist.