# 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.