v1.63: ESLint in CI and a syntax check of every script

.github/workflows/test.yml

@@ -24,6 +24,7 @@ jobs:
           node-version: 22
           cache: npm
       - run: npm ci
+      - run: npm run lint
       - run: npx playwright install --with-deps chromium
       - run: npm test -- --workers=2
       - uses: actions/upload-artifact@v4

CLAUDE.md

@@ -12,7 +12,8 @@ A browser-only tool for Australian electricity bills (Amber Electric vs other pl
 ```bash
 npm install
 npm run build:css     # after changing any Tailwind class in index.html or js/; commit css/app.css
-npm run test:unit     # Node unit checks (fast)
+npm run lint          # ESLint: unused or shadowed variables, undefined names, house style
+npm run test:unit     # Node unit checks (fast), starting with a syntax check of every js/*.js
 npm test              # unit checks + Playwright (starts scripts/static-server.js on :4173)
 npx playwright test tests/planner.spec.js --workers=2   # one spec
 npm run serve         # http://127.0.0.1:4173
@@ -40,5 +41,5 @@ If Playwright can't find its browser, point it at an installed Chromium with `la
    - `js/constants.js`: `Amber.APP_VERSION`;
    - `tests/unit-time.js`: the version assert.
 2. Update `README.md` and `docs/` if behaviour or structure changed.
-3. Run `npm run build:css` if classes changed, then `npm test`.
-4. Open a pull request. Merge only after the GitHub "Tests" check passes (GitHub Pages serves `main`).
+3. Run `npm run build:css` if classes changed, then `npm run lint` and `npm test`.
+4. Open a pull request. Merge only after the GitHub "Tests" check passes: it runs the lint and the tests (GitHub Pages serves `main`).

README.md

@@ -92,8 +92,9 @@ The solar and battery planner. From the last 12 months of meter readings (Amber'
 ## For developers
 
 ```bash
-npm install          # Tailwind CLI and Playwright
+npm install          # Tailwind CLI, ESLint and Playwright
 npm run build:css    # css/input.css -> css/app.css (commit the result)
+npm run lint         # ESLint (eslint.config.js)
 npm test             # unit checks, then Playwright against a local static server
 npm run test:unit    # unit checks only
 npm run serve        # http://127.0.0.1:4173

docs/ARCHITECTURE.md

@@ -25,7 +25,9 @@ scripts/update-prices.js Downloads AEMO prices into data/prices (run daily by a
 scripts/update-solar.js  Downloads PVGIS weather into data/solar (run by hand)
 tests/unit-*.js          Node unit checks (no framework: assert + console)
 tests/*.spec.js          Playwright end-to-end tests (Chromium)
-.github/workflows/       test.yml (tests on PRs and main), update-prices.yml (daily price refresh)
+eslint.config.js         ESLint settings (npm run lint)
+eslint-rules/            The project's own ESLint rules (no-extra-arguments)
+.github/workflows/       test.yml (lint and tests on PRs and main), update-prices.yml (daily price refresh)
 chart.umd.min.js, chartjs-plugin-zoom.min.js, pdf.min.js, pdf.worker.min.js   Vendored libraries
 ```
 
@@ -218,14 +220,15 @@ Saved JSON is read through `storedJson` / `storedCustomPlans` in `app.js`: a val
 ## Build, test and CI
 
 - `npm run build:css` rebuilds `css/app.css`. Tailwind scans `index.html` and `js/**/*.js` for class names, and `tailwind.config.js` safelists classes that are only built dynamically. Commit `css/app.css` after changing classes.
-- **Unit checks**: `tests/unit-*.js`, run with plain `node` (`npm run test:unit`). Each prints `ok:` / `FAIL:` lines and exits non-zero on failure. `unit-planner.js` holds the parity checks: the planner's cost equals Compare's, battery energy balances, finance and ratings.
-- **End-to-end**: `tests/*.spec.js` with Playwright (Chromium) against `scripts/static-server.js` on port 4173 (`playwright.config.js`, `reuseExistingServer: true`). Tests mock Amber's API with `page.route('**/api.amber.com.au/**', …)` and build NEM12 files in code. About 54 tests take about 45 s with two workers.
-- **CI**: `.github/workflows/test.yml` runs `npm ci`, installs Chromium and runs `npm test -- --workers=2` on every pull request and on pushes to `main`. Failed runs upload `test-results/`.
+- **Lint**: `npm run lint` runs ESLint 10 with `eslint.config.js` (a flat config). The files in `js/` are checked as classic browser scripts, and the unit checks, scripts and specs as CommonJS. On top of ESLint's recommended set it reports unused variables, shadowed names, `var`, `==` and a `let` that never changes, and holds the house style (4-space indent, single quotes, semicolons, from `@stylistic/eslint-plugin`). One rule is the project's own: `local/no-extra-arguments` (`eslint-rules/no-extra-arguments.js`) reports a call that passes more arguments than the function takes, for functions and `Amber.*` functions defined in the same file. Rules are tuned to the code rather than the code restyled: the tests keep their own alignment of long asserts, and `preserve-caught-error` is off because network errors are replaced on purpose by plain messages.
+- **Unit checks**: `tests/unit-*.js`, run with plain `node` (`npm run test:unit`). Each prints `ok:` / `FAIL:` lines and exits non-zero on failure. `unit-syntax.js` runs first: it parses every `js/*.js` (no other unit script loads the DOM files such as `help.js` and `app.js`) and checks that `index.html` loads each of them once, with `app.js` last. `unit-planner.js` holds the parity checks: the planner's cost equals Compare's, battery energy balances, finance and ratings.
+- **End-to-end**: `tests/*.spec.js` with Playwright (Chromium) against `scripts/static-server.js` on port 4173 (`playwright.config.js`, `reuseExistingServer: true`). Tests mock Amber's API with `page.route('**/api.amber.com.au/**', …)` and build NEM12 files in code. About 66 tests take about a minute with two workers.
+- **CI**: `.github/workflows/test.yml` runs `npm ci`, `npm run lint`, installs Chromium and runs `npm test -- --workers=2` on every pull request and on pushes to `main`. Failed runs upload `test-results/`.
 
 ## Releases
 
 Each change ships as a version:
 
 1. Bump the version in three places: `index.html` (the `<title>`, the "Version X." line in the header, and every `?v=` query string), `js/constants.js` (`Amber.APP_VERSION`), and `tests/unit-time.js` (the version assert).
 2. `npm run build:css` if classes changed; update the README and these docs if behaviour changed.
-3. Run `npm test`, open a pull request, and merge once the Tests check passes. GitHub Pages serves `main`.
+3. Run `npm run lint` and `npm test`, open a pull request, and merge once the Tests check passes. GitHub Pages serves `main`.

docs/FRONTEND.md

@@ -127,6 +127,7 @@ The app is written for everyday Australian householders:
 
 ```bash
 npm run build:css
+npm run lint
 npm test                       # or: npx playwright test tests/planner.spec.js
 npm run serve                  # then open http://127.0.0.1:4173
 ```

docs/REVIEW.md

@@ -7,11 +7,12 @@ Notes for anyone reviewing this codebase: what it must keep doing, where mistake
 ```bash
 npm install
 npm run build:css
-npm test            # ~10 unit scripts, then ~54 Playwright tests (~45 s with --workers=2)
+npm run lint        # ESLint
+npm test            # ~11 unit scripts, then ~66 Playwright tests (~60 s with --workers=2)
 npm run serve       # http://127.0.0.1:4173
 ```
 
-Without an Amber API key, use the "Not with Amber?" path on Start with a NEM12 file. The tests build them in code (`nem12Year()`, `nem12Battery()` in `tests/planner.spec.js`). CI runs the same `npm test` on every pull request (`.github/workflows/test.yml`).
+Without an Amber API key, use the "Not with Amber?" path on Start with a NEM12 file. The tests build them in code (`nem12Year()`, `nem12Battery()` in `tests/planner.spec.js`). CI runs `npm run lint` and the same `npm test` on every pull request (`.github/workflows/test.yml`).
 
 ## Invariants: what must stay true
 
@@ -44,7 +45,7 @@ These are the properties the tests guard and that users rely on. A change that b
 
 - **`app.js` is a monolith.** Tabs, plan form, results, imports, bill check, Free? and settings live in one IIFE sharing a `state` object. Natural seams: the plan form (`createPlanObjectFromForm`, `applyPlanTemplate`, TOU windows), results rendering, imports (CSV, NEM12, local sites), the bill check, the Free? tab and settings.
 - **HTML as strings.** Most results are template strings set with `innerHTML`, with table markup repeated (`th`/`td` helpers exist only in `planner-ui.js`). It's safe while everything is escaped, but verbose and easy to make inconsistent.
-- **No linter, formatter or type checking.** There's no ESLint or Prettier config; the style is 4-space indentation, single quotes and semicolons. JSDoc comments exist on many engine functions but not throughout.
+- **No formatter or type checking.** ESLint (`npm run lint`, run in CI) catches unused and shadowed variables, undefined names and calls with too many arguments within a file, and holds the style (4-space indentation, single quotes, semicolons). There's no Prettier, and nothing checks types or calls between files. JSDoc comments exist on many engine functions but not throughout.
 - **Home-made unit test harness.** `tests/unit-*.js` use their own `assert` and print `ok:`/`FAIL:`. They work, but give no per-test isolation or coverage.
 - **End-to-end tests depend on IDs and wording.** Many assertions match visible text, so copy edits need test edits too.
 - **Yearly maintenance is manual.** Each 1 July: the default offers and VDO in `templates.js`, Amber's network tariffs in `networks.js` (FY2026-27 today), Amber's default fixed charges and `AMBER_RATES_CHANGE_DATE` in `constants.js`, rebate and hardware defaults in `PLANNER_FINANCE_DEFAULTS`, and `TEMPLATES_AS_AT`. Market offers drift during the year.
@@ -57,7 +58,7 @@ These are the properties the tests guard and that users rely on. A change that b
 
 1. Is it worth splitting `app.js` along the seams above before a frontend refresh, so the refresh doesn't have to touch pricing code?
 2. Should HTML building move to small render helpers (or `<template>` elements in `index.html`) to cut repetition, while staying build-free and within the CSP?
-3. Would a lint/format setup (ESLint + Prettier, run in CI) be welcome, given the no-build constraint?
+3. ESLint now runs in CI. Would a formatter (Prettier) or type checking of the JSDoc (`tsc --checkJs`) be welcome too, given the no-build constraint?
 4. Is the planner's battery heuristic good enough, or should it be compared against a simple linear-programming optimum for a few test days to measure how optimistic it is?
 5. Should the Solar+? search move to a Web Worker (it runs on the main thread today, yielding with `setTimeout(0)`)? The CSP already allows `worker-src 'self'`.
 6. Should the app say explicitly that it's independent of Amber Electric (see FRONTEND.md)?
@@ -67,6 +68,7 @@ These are the properties the tests guard and that users rely on. A change that b
 
 | Test | Covers |
 |---|---|
+| `unit-syntax.js` | Every `js/*.js` parses as a classic script; `index.html` loads each one once, `app.js` last |
 | `unit-time.js` | Dates, NEM vs local time, TOU windows, app version |
 | `unit-bill.js`, `unit-billcheck.js` | Bill reading and line-by-line checks |
 | `unit-stats.js` | More Stats |

eslint-rules/no-extra-arguments.js

@@ -0,0 +1,104 @@
+'use strict';
+
+/*
+ * A local ESLint rule: a call that passes more arguments than the function takes. JavaScript
+ * drops the extras silently, so they are usually a leftover from an older signature (or a sign
+ * that the caller expects something the function doesn't do).
+ *
+ * It only looks at calls it can be sure about, inside one file:
+ *   - a function declared in that file (`function f(a) {}` or `const f = (a) => {}`), called as `f(…)`;
+ *   - a function put on the namespace in that file (`Amber.f = function (a) {}`), called as `Amber.f(…)`.
+ * Functions that take `...rest` or read `arguments` are left alone, and so are calls with `...spread`.
+ */
+
+function isFunction(node) {
+    return !!node && (node.type === 'FunctionDeclaration' || node.type === 'FunctionExpression' || node.type === 'ArrowFunctionExpression');
+}
+
+module.exports = {
+    meta: {
+        type: 'problem',
+        docs: { description: 'disallow calling a function with more arguments than it takes' },
+        schema: [{
+            type: 'object',
+            properties: { namespaces: { type: 'array', items: { type: 'string' } } },
+            additionalProperties: false
+        }],
+        messages: {
+            extra: '\'{{name}}\' takes {{takes}} argument{{s}} but is called with {{given}}.'
+        }
+    },
+    create(context) {
+        const sourceCode = context.sourceCode;
+        const namespaces = new Set((context.options[0] && context.options[0].namespaces) || []);
+        const members = new Map();      // 'Amber.name' -> function node, or null when assigned twice
+        const memberCalls = [];
+
+        /** How many arguments the function takes, or null when it takes any number. */
+        function arity(fn) {
+            if (fn.params.some((p) => p.type === 'RestElement')) return null;
+            if (fn.type !== 'ArrowFunctionExpression') {
+                const scope = sourceCode.scopeManager.acquire(fn);
+                const args = scope && scope.set.get('arguments');
+                if (args && args.references.length) return null;
+            }
+            return fn.params.length;
+        }
+
+        /** The one function a name always refers to, or null when that can't be known. */
+        function localFunction(node) {
+            let scope = sourceCode.getScope(node);
+            let variable = null;
+            while (scope && !variable) {
+                variable = scope.set.get(node.name) || null;
+                scope = scope.upper;
+            }
+            if (!variable || variable.defs.length !== 1) return null;
+            const def = variable.defs[0];
+            if (variable.references.some((ref) => ref.isWrite() && !ref.init)) return null;
+            if (def.type === 'FunctionName' && def.node.type === 'FunctionDeclaration') return def.node;
+            if (def.type === 'Variable' && def.node.id.type === 'Identifier' && isFunction(def.node.init)) return def.node.init;
+            return null;
+        }
+
+        function memberName(node) {
+            if (node.type !== 'MemberExpression' || node.computed) return null;
+            if (node.object.type !== 'Identifier' || !namespaces.has(node.object.name)) return null;
+            return `${node.object.name}.${node.property.name}`;
+        }
+
+        function check(call, fn, name) {
+            if (call.arguments.some((a) => a.type === 'SpreadElement')) return;
+            const takes = arity(fn);
+            if (takes == null || call.arguments.length <= takes) return;
+            context.report({
+                node: call.arguments[takes],
+                messageId: 'extra',
+                data: { name, takes, s: takes === 1 ? '' : 's', given: call.arguments.length }
+            });
+        }
+
+        return {
+            AssignmentExpression(node) {
+                const name = memberName(node.left);
+                if (!name) return;
+                members.set(name, members.has(name) || node.operator !== '=' || !isFunction(node.right) ? null : node.right);
+            },
+            CallExpression(node) {
+                if (node.callee.type === 'Identifier') {
+                    const fn = localFunction(node.callee);
+                    if (fn) check(node, fn, node.callee.name);
+                    return;
+                }
+                const name = memberName(node.callee);
+                if (name) memberCalls.push({ node, name });
+            },
+            'Program:exit'() {
+                memberCalls.forEach(({ node, name }) => {
+                    const fn = members.get(name);
+                    if (fn) check(node, fn, name);
+                });
+            }
+        };
+    }
+};

eslint.config.js

@@ -0,0 +1,83 @@
+'use strict';
+
+/*
+ * ESLint for the code as it is: classic browser scripts (each js/*.js is an IIFE adding to
+ * window.Amber, no modules, no build step), Node unit checks and scripts, and Playwright specs.
+ * It looks for real problems (unused or shadowed variables, unreachable code, undefined names)
+ * and holds the house style: 4-space indent, single quotes, semicolons. Run: npm run lint
+ */
+const js = require('@eslint/js');
+const globals = require('globals');
+const stylistic = require('@stylistic/eslint-plugin');
+const noExtraArguments = require('./eslint-rules/no-extra-arguments.js');
+
+// The project's own rules (eslint-rules/).
+const local = { rules: { 'no-extra-arguments': noExtraArguments } };
+
+const rules = {
+    ...js.configs.recommended.rules,
+    'no-unused-vars': ['error', { args: 'after-used', caughtErrors: 'none' }],
+    'no-shadow': ['error', { builtinGlobals: false, hoist: 'functions' }],
+    'no-unreachable': 'error',
+    'local/no-extra-arguments': ['error', { namespaces: ['Amber'] }],
+    // An error caught from the network is replaced on purpose by a plain message for the householder.
+    'preserve-caught-error': 'off',
+    'no-var': 'error',
+    'prefer-const': ['error', { destructuring: 'all' }],
+    eqeqeq: ['error', 'always', { null: 'ignore' }],
+    '@stylistic/indent': ['error', 4, { SwitchCase: 1 }],
+    '@stylistic/quotes': ['error', 'single', { avoidEscape: true, allowTemplateLiterals: 'always' }],
+    '@stylistic/semi': ['error', 'always']
+};
+
+module.exports = [
+    {
+        // Vendored libraries, built CSS and data are not ours to lint.
+        ignores: ['node_modules/**', '*.min.js', 'css/**', 'data/**', 'test-results/**', 'playwright-report/**']
+    },
+    {
+        files: ['**/*.js'],
+        plugins: { '@stylistic': stylistic, local },
+        linterOptions: { reportUnusedDisableDirectives: 'error' },
+        rules
+    },
+    {
+        // The app: classic scripts sharing window.Amber. The same files are `require`d by the Node
+        // unit checks, hence `module` and `globalThis`. Chart and pdfjsLib are the vendored libraries.
+        files: ['js/**/*.js'],
+        languageOptions: {
+            ecmaVersion: 2022,
+            sourceType: 'script',
+            globals: {
+                ...globals.browser,
+                Chart: 'readonly',
+                pdfjsLib: 'readonly',
+                module: 'readonly'
+            }
+        }
+    },
+    {
+        files: ['tests/unit-*.js', 'scripts/**/*.js', 'eslint-rules/**/*.js', '*.config.js'],
+        languageOptions: {
+            ecmaVersion: 2022,
+            sourceType: 'commonjs',
+            globals: { ...globals.node }
+        }
+    },
+    {
+        // Playwright specs run in Node, with page.evaluate callbacks that run in the page.
+        files: ['tests/**/*.spec.js'],
+        languageOptions: {
+            ecmaVersion: 2022,
+            sourceType: 'commonjs',
+            globals: { ...globals.node, ...globals.browser, Amber: 'readonly', Chart: 'readonly' }
+        }
+    },
+    {
+        // The tests line up long assert conditions and tables of cases their own way.
+        files: ['tests/**/*.js'],
+        rules: {
+            '@stylistic/indent': ['error', 4, { SwitchCase: 1, ArrayExpression: 'off', CallExpression: { arguments: 'off' } }]
+        }
+    }
+];