diff --git a/docs/plans/2026-02-26-script-js-cleanup-plan.md b/docs/plans/2026-02-26-script-js-cleanup-plan.md new file mode 100644 index 00000000..2debfee8 --- /dev/null +++ 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 = `