Cookie Legacy Code Review

← Back to article

Repository: https://github.com/matthewdeaves/cookie Commit: cfaafa8 Date: 2026-01-25 Scope: apps/legacy/static/legacy/js/ (ES5 JavaScript for iOS 9 support)

This review was generated by Claude Opus 4.5 through direct analysis of the codebase and metrics collected by the CI dashboard (GitHub Actions: ESLint, duplication detection).

This review examines the legacy ES5 JavaScript frontend built for iOS 9 compatibility. The constraints are different from modern JavaScript - no modules, no async/await, no modern build tools. But the architectural patterns are strikingly similar to the modern React frontend, suggesting the same underlying planning issues.

---

# Executive Summary

The legacy codebase is 4,623 lines of vanilla ES5 JavaScript split across 9 page modules. Code structure mirrors the modern frontend's problems: god components, duplicated utilities, and monolithic files that mix concerns. The 6.67% duplication metric is actually conservative - it only measures text duplication, not the architectural duplication evident in the code.

Critical issues:

• detail.js god component (1,275 lines, manages recipe display, favorites, collections, AI scaling, remix, tips, polling, play mode)
• settings.js god component (1,100 lines, manages 6 tabs with API keys, prompts, sources, profiles, database reset)
• Global namespace pollution (8+ functions exposed on window for inline onclick handlers)
• No shared utilities (escapeHtml() reimplemented 5 times across files)

Moderate issues:

• Event listener attachment anti-pattern (16+ loops attaching listeners instead of delegation)
• Large HTML string concatenation (no templating, maintenance nightmare)
• Callback hell in async operations (nested AJAX callbacks 3-4 levels deep)
• Variable shadowing risk (loop variables reused across 16+ loops in settings.js)
• DOM querying inefficiency (repeated queries with no caching)

ES5 constraints:

• No module system makes utility sharing impossible without global namespace pollution or async module loading
• No template literals makes HTML generation verbose
• No Promises means callback nesting is unavoidable
• No const/let means variable shadowing risk is higher

---

# 1. Architecture & Structure

# File Size Distribution

Pattern: Two files (detail.js and settings.js) contain half the codebase. This mirrors the modern frontend where Settings.tsx (1,314 lines) and RecipeDetail.tsx (739 lines) dominate.

# God Component: detail.js (1,275 lines)

Location: apps/legacy/static/legacy/js/pages/detail.js

What it manages:

• Recipe display with 4 tabs (ingredients, instructions, nutrition, tips)
• Favorite toggling
• Collection management (add/remove recipe)
• Serving adjustment with AI-powered ingredient scaling
• Remix functionality with AI suggestions
• Tips generation via AI
• Polling for async AI tips
• Navigation to play mode
• Modal management for collections and remix
• Error handling for AI features
• Form state for serving input and remix text area

State variables (defined in module scope):

var currentRecipeData = null;
var currentIngredients = null;
var originalIngredients = null;
var tipsPollingInterval = null;
var favoriteBtn = null;
var collectionsContainer = null;
// ... 20+ more variables

Problem: Violates single responsibility principle. One file handles everything from basic recipe display to complex AI polling and modal interactions. Testing a single feature requires understanding the entire 1,275-line file.

Example complexity - serving adjustment spans 50+ lines mixing DOM manipulation, validation, AI calls, error handling, and state updates without clear separation.

# God Component: settings.js (1,100 lines)

Location: apps/legacy/static/legacy/js/pages/settings.js

What it manages:

• Tab navigation (6 tabs: general, prompts, sources, selectors, users, danger)
• API key validation and persistence
• Prompt editing for 4 different prompt types (discover, remix, scale, tips)
• Source enable/disable toggles (individual and bulk)
• CSS selector editing and testing (individual and batch)
• Profile management (list, delete with preview modal)
• Database reset (two-step confirmation with text input validation)

Initialization pattern (lines 130-132, repeated 16+ times):

for (var i = 0; i < testSourceBtns.length; i++) {
    testSourceBtns[i].addEventListener('click', function(e) {
        // handler logic
    });
}

Problem: Attaching listeners in loops instead of using event delegation. Every tab uses this pattern:

• Lines 143-146: test batch source buttons
• Lines 259-261: toggle source buttons
• Lines 433-436: prompt edit buttons
• Lines 445-448: prompt save buttons
• Lines 471-473: prompt toggle buttons
• Lines 494-496: individual source test buttons
• Lines 524-526: individual source toggle buttons
• Lines 548-550: batch test buttons
• Lines 596-598: edit selector buttons
• Lines 601-603: save selector buttons
• Lines 606-608: test individual selector buttons
• Lines 611-613: test batch selector buttons
• Lines 758-761: delete profile buttons
• Lines 812-814: reset modal buttons
• Lines 847-849: execute reset buttons

Better approach: Single delegated listener on the container:

// Instead of 16 loops
document.addEventListener('click', function(e) {
    if (e.target.matches('[data-action="toggle-source"]')) {
        handleToggleSource(e);
    }
    // ... other actions
});

This would reduce 16 loops to 1 event listener, improve performance, and handle dynamically added elements automatically.

# Global Namespace Pollution

Location: apps/legacy/static/legacy/js/pages/settings.js lines 1077-1097

Problem: Functions exposed globally for inline onclick handlers:

window.closeDeleteModal = closeDeleteModal;
window.executeDeleteProfile = executeDeleteProfile;
window.showResetModal = showResetModal;
window.showResetStep1 = showResetStep1;
window.showResetStep2 = showResetStep2;
window.closeResetModal = closeResetModal;
window.executeReset = executeReset;
window.closeTestModal = closeTestModal;
window.renderSourceCard = renderSourceCard;

Why it exists: HTML uses inline event handlers:

<button onclick="closeDeleteModal()">Cancel</button>
<button onclick="executeDeleteProfile()">Confirm</button>

Risk: Global namespace collisions. If another page module defines closeDeleteModal, the last one loaded wins. No warning, just silent bugs.

Better approach: Use data attributes and delegated listeners (as suggested for the loop pattern above). Removes need for global exposure entirely.

---

# 2. Code Duplication

# escapeHtml() - Reimplemented 5 Times

Locations:

• detail.js:1086
• home.js:273
• play.js:569
• search.js:387
• settings.js:772

Implementation (identical in all 5 files):

function escapeHtml(text) {
    var div = document.createElement('div');
    div.textContent = text;
    return div.innerHTML;
}

Why it's duplicated: ES5 has no module system. Each page file is self-contained. Options are:

1. Duplicate the function (current approach)
2. Put it in global scope (namespace pollution)
3. Use async module loading like RequireJS (adds complexity)

Impact: This is why the 6.67% duplication metric is actually conservative. The architectural duplication (god components, repeated patterns) isn't captured by text duplication detection.

# handleTabClick() - Reimplemented 3 Times

Locations:

• detail.js:264
• home.js:96
• settings.js:138

Pattern (similar structure in all 3):

function handleTabClick(e) {
    e.preventDefault();
    var clickedTab = e.target.closest('.tab-btn');
    if (!clickedTab) return;

    var allTabs = document.querySelectorAll('.tab-btn');
    var allPanels = document.querySelectorAll('.tab-panel');

    for (var i = 0; i < allTabs.length; i++) {
        allTabs[i].classList.remove('active');
    }
    for (var i = 0; i < allPanels.length; i++) {
        allPanels[i].classList.remove('active');
    }

    clickedTab.classList.add('active');
    var targetPanel = document.getElementById(clickedTab.dataset.tab);
    if (targetPanel) targetPanel.classList.add('active');
}

Problem: Same DOM manipulation logic repeated across 3 files. Each file implements tabs independently.

Why it happened: No shared component system in ES5. Each screen handles its own tab behaviour.

# init() Function - Present in 9 Page Modules

Each page file exports an init() function that:

1. Queries and caches DOM elements
2. Attaches event listeners
3. Loads initial data

Pattern (repeated 9 times):

function init() {
    // Cache DOM elements
    var apiKeyInput = document.getElementById('api-key-input');
    var testKeyBtn = document.getElementById('test-key-btn');
    // ... 20+ more queries

    // Attach event listeners
    testKeyBtn.addEventListener('click', handleTestKey);
    // ... 30+ more listeners

    // Load data
    loadInitialData();
}

Problem: Every page follows this pattern but implements it from scratch. No shared initialization framework.

---

# 3. HTML Generation

# Large String Concatenation

Location: settings.js:450-464

Example (building source card HTML):

var toggleIcon = source.is_enabled
    ? '<svg class="toggle-icon toggle-on" xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2"><circle cx="12" cy="12" r="10"></circle><path d="M9 12l2 2 4-4"></path></svg>'
    : '<svg class="toggle-icon toggle-off" xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2"><circle cx="12" cy="12" r="10"></circle></svg>';

html += '<div class="source-card" data-source-id="' + source.id + '">';
html += '  <div class="source-header">';
html += '    <h3>' + escapeHtml(source.domain) + '</h3>';
html += '    <button type="button" class="btn-toggle" data-action="toggle-source" data-source-id="' + source.id + '">';
html += '      ' + toggleIcon;
html += '    </button>';
html += '  </div>';
// ... 20+ more lines of concatenation

Problems:

1. No syntax highlighting or editor support for HTML
2. Easy to forget closing tags
3. Mixing data (source.id) with markup makes it hard to spot XSS risks
4. Changing UI requires editing long string concatenation
5. No template reuse - each card is built from scratch

ES5 constraints: No template literals, no JSX, no modern templating engines client-side.

Better approach (within ES5 constraints): Use a micro-templating function or HTML5 template elements:

// Define template once
var sourceCardTemplate = document.getElementById('source-card-template');

function renderSourceCard(source) {
    var clone = sourceCardTemplate.content.cloneNode(true);
    clone.querySelector('.source-name').textContent = source.domain;
    clone.querySelector('[data-action="toggle-source"]').dataset.sourceId = source.id;
    // ... set other values
    return clone;
}

This separates markup (in HTML) from logic (in JavaScript), making both easier to maintain.

---

# 4. Async Operations

# Callback Hell

Location: detail.js

Example (collection management with nested callbacks):

Cookie.ajax.post('/api/collections/', { name: name }, function(err, collection) {
    if (err) {
        Cookie.toast.error('Failed to create collection');
        return;
    }

    // Nested callback level 1
    Cookie.ajax.post('/api/collections/' + collection.id + '/recipes/',
        { recipe_id: recipeId },
        function(err2) {
            if (err2) {
                Cookie.toast.error('Failed to add recipe');
                return;
            }

            // Nested callback level 2
            Cookie.ajax.get('/api/collections/', function(err3, collections) {
                if (err3) {
                    Cookie.toast.error('Failed to reload collections');
                    return;
                }

                // Finally update UI
                updateCollectionsUI(collections);
            });
        }
    );
});

Problem: Three levels of nesting for a single user action (create collection, add recipe, refresh list). Error handling repeated at each level. Logic flow is hard to follow.

ES5 constraint: No Promises, no async/await. Callbacks are the only option.

Better approach (within ES5 constraints): Extract named functions to flatten nesting:

function createCollection(name, recipeId) {
    Cookie.ajax.post('/api/collections/', { name: name }, function(err, collection) {
        if (err) return handleError('Failed to create collection');
        addRecipeToCollection(collection.id, recipeId);
    });
}

function addRecipeToCollection(collectionId, recipeId) {
    Cookie.ajax.post('/api/collections/' + collectionId + '/recipes/',
        { recipe_id: recipeId },
        function(err) {
            if (err) return handleError('Failed to add recipe');
            refreshCollections();
        }
    );
}

function refreshCollections() {
    Cookie.ajax.get('/api/collections/', function(err, collections) {
        if (err) return handleError('Failed to reload collections');
        updateCollectionsUI(collections);
    });
}

Still callbacks, but each function has a single level of nesting. Logic flow is clearer.

---

# 5. DOM Manipulation

# Repeated Queries Without Caching

Location: settings.js:338-339 and throughout

Example (querying same card element multiple times):

card.querySelector('[data-field="system_prompt"]').value = prompt.system_prompt;
card.querySelector('[data-field="user_prompt_template"]').value = prompt.user_prompt_template;
card.querySelector('[data-field="model"]').value = prompt.model;
card.querySelector('[data-field="is_enabled"]').checked = prompt.is_enabled;

Problem: Each querySelector traverses the DOM. For 4 fields, that's 4 separate DOM queries on the same parent.

Better approach:

var fields = {
    systemPrompt: card.querySelector('[data-field="system_prompt"]'),
    userPrompt: card.querySelector('[data-field="user_prompt_template"]'),
    model: card.querySelector('[data-field="model"]'),
    enabled: card.querySelector('[data-field="is_enabled"]')
};

fields.systemPrompt.value = prompt.system_prompt;
fields.userPrompt.value = prompt.user_prompt_template;
fields.model.value = prompt.model;
fields.enabled.checked = prompt.is_enabled;

Query once, reuse references. Reduces DOM traversal by 75%.

# Variable Shadowing Risk

Location: settings.js throughout

Problem: Loop variable i used in 16+ loops within the same module scope:

for (var i = 0; i < testSourceBtns.length; i++) { ... }
// ... 50 lines later
for (var i = 0; i < toggleSourceBtns.length; i++) { ... }
// ... 100 lines later
for (var i = 0; i < sources.length; i++) { ... }

Why it's risky: var is function-scoped, not block-scoped. All these i variables are actually the same variable being reused. If loops are nested (they sometimes are), inner loop clobbers outer loop's counter, causing bugs.

ES5 constraint: No let or const for block scoping.

Mitigation: Use IIFEs to create scope or use different variable names (i, j, k, idx, etc.) to make shadowing obvious.

---

# 6. ESLint Warnings (17 warnings, C rating)

# Complexity Rules

Configuration (.eslintrc.js):

rules: {
    'complexity': ['warn', 15],              // max cyclomatic complexity
    'max-depth': ['warn', 4],                // max nesting depth
    'max-lines-per-function': ['warn', 100], // max lines per function
    'max-nested-callbacks': ['warn', 4],     // max callback nesting
}

Where rules are breached:

• settings.js functions regularly exceed 100 lines (setupSelectorsTab, renderSourceCard)
• detail.js has functions with complexity > 15 (serving adjustment, remix handling)
• Callback nesting reaches 3-4 levels in async operations

# Quality Rules

Configuration:

'no-unused-vars': ['warn', { args: 'none' }],  // warn on unused variables
'no-undef': 'error',                           // error on undefined globals
'no-shadow': 'warn',                           // warn on variable shadowing
'eqeqeq': ['warn', 'smart'],                   // warn on == instead of ===

Issues caught:

• Unused variables (likely from refactoring)
• Undefined globals (missing declarations)
• Variable shadowing (the loop variable i reuse pattern)

Impact: The 17 warnings are symptoms of the larger architectural issues. Fixing god components and extracting functions would reduce complexity naturally.

---

# 7. What to Improve

# Critical Fixes

1. Extract settings.js tabs into separate modules

Current: 1,100-line file with 6 tabs inline Target: 6 modules (settings-general.js, settings-prompts.js, etc.) loaded on demand

Benefits:

• Reduces per-file complexity
• Makes tab logic testable in isolation
• Easier to understand and modify individual tabs

Trade-off: Need async module loading (RequireJS or similar), adds build complexity

2. Extract detail.js features into separate modules

Current: 1,275-line file handling display, favorites, collections, AI features, polling Target: Split into detail-display.js, detail-ai.js, detail-collections.js

Benefits: Same as settings.js modularisation

3. Create shared utilities module

Extract duplicated functions into Cookie.utils:

• escapeHtml() (duplicated 5 times)
• handleTabClick() (duplicated 3 times)
• formatTime() (similar implementations in multiple files)

Load utilities before page modules, expose on Cookie namespace.

# Moderate Fixes

4. Replace event listener loops with delegation

Current: 16+ loops in settings.js attaching individual listeners Target: Single delegated listener per container

Example:

// Replace all 16 loops with:
document.getElementById('settings-container').addEventListener('click', function(e) {
    var action = e.target.dataset.action;
    if (!action) return;

    var handlers = {
        'toggle-source': handleToggleSource,
        'test-source': handleTestSource,
        'edit-prompt': handleEditPrompt,
        // ... all actions
    };

    var handler = handlers[action];
    if (handler) handler(e);
});

Benefits:

• Reduces listener count from 100+ to 1
• Handles dynamically added elements automatically
• Removes need for global function exposure

5. Replace HTML string concatenation with template elements

Move HTML structure from JavaScript strings to HTML5 <template> elements. Clone and populate templates instead of building strings.

Benefits:

• Editor syntax highlighting for HTML
• Easier to spot missing closing tags
• Clearer separation of markup and logic

6. Flatten callback nesting

Extract nested callbacks into named functions (example shown in Async Operations section).

Benefits:

• Easier to follow logic flow
• Reduces indentation depth
• Makes error handling consistent

# Process Fixes

7. Specify module boundaries in plans

Instead of "build settings screen", write "build settings screen with separate modules for each tab: general (API keys), prompts (4 types), sources (enable/disable), selectors (CSS editing), users (profile CRUD), danger (reset)".

This prevents 1,100-line files from being created in the first place.

8. Require event delegation pattern

Plans for ES5 JavaScript should specify: "Use event delegation for dynamic elements. Attach single listener to container, check event.target for specific elements."

This prevents the 16-loop pattern.

9. Create shared utilities first

Before building page modules, create Cookie.utils with common functions (escapeHtml, formatTime, etc.). Reference it in page module tasks: "Use Cookie.utils.escapeHtml() for HTML escaping."

This prevents duplication from the start.

---

# Summary

The 6.67% duplication metric is honest but incomplete. It captures text duplication (escapeHtml reimplemented 5 times) but not architectural duplication (god components, repeated patterns, no shared abstractions).

The ES5 constraints are real - no modules, no Promises, no template literals. But within those constraints, better patterns exist:

• God components can be split into smaller modules with async loading
• Event listener loops can be replaced with delegation
• HTML string concatenation can use template elements
• Callback hell can be flattened with named functions
• DOM queries can be cached

The legacy code mirrors the modern frontend's problems: settings.js (1,100 lines) matches Settings.tsx (1,314 lines), detail.js (1,275 lines) matches RecipeDetail.tsx (739 lines). The planning issues are the same regardless of technology choice.