From 228964b9088f688b1b9b066be79e601808197139 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Date: Thu, 1 Oct 2026 20:37:47 +0200 Subject: [PATCH 1/2] (test): run the shipped app.js code in five replica tests indexing-banner, confirm-and-stop-session, open-session-terminal, search-perf and running-indicators asserted against a copy of the logic written in the test, so a change to public/app.js could not fail them. They now load the real functions through test/app-source.js into the jsdom window of dom-setup.js, and each goes red when the production line it guards is broken. Closes #171 --- .ai/shared-guidelines.md | 1 + test/app-source.js | 87 ++++ test/confirm-and-stop-session.test.js | 219 ++++------ test/dom-setup.js | 1 + test/indexing-banner.test.js | 169 ++++---- test/open-session-terminal.test.js | 239 +++++------ test/running-indicators.test.js | 587 ++++++++------------------ test/search-perf.test.js | 389 ++++++++--------- 8 files changed, 721 insertions(+), 971 deletions(-) create mode 100644 test/app-source.js diff --git a/.ai/shared-guidelines.md b/.ai/shared-guidelines.md index 339f7610..29b91aa3 100644 --- a/.ai/shared-guidelines.md +++ b/.ai/shared-guidelines.md @@ -144,6 +144,7 @@ These exist on `devsuitup/switchboard` main but not on `doctly/switchboard` main - `node:test` runner via `npm test` / `task test`. - Renderer tests use jsdom via `test/dom-setup.js` + `vm.runInContext` to evaluate `public/*.js` in isolation. +- `public/app.js` cannot be evaluated whole. To test one of its functions, load the shipped source with `loadAppFunctions` from `test/app-source.js` into the window `setupSidebarDom()` returns, and stub only its outside edges. Never copy the function into the test. - Pitfall: `installSpies: false` is required when the eval defines functions you also spy on — function declarations from eval overwrite property spies. - Always test in the **primary checkout** (`C:\Serveur\switchboard` on this machine), not inside `.claude/worktrees/agent-*`. Worktrees may have incomplete `node_modules` and produce false negatives on tests that require native modules (e.g. `morphdom`). diff --git a/test/app-source.js b/test/app-source.js new file mode 100644 index 00000000..da3a73c5 --- /dev/null +++ b/test/app-source.js @@ -0,0 +1,87 @@ +'use strict'; + +// Loads functions out of public/app.js into a jsdom window's VM context. +// app.js cannot be evaluated whole (module scope builds real panels and +// terminals), so a test names the top-level functions and one-line +// declarations it needs and gets the shipped source of exactly those. + +const fs = require('node:fs'); +const path = require('node:path'); +const vm = require('node:vm'); + +const APP_PATH = path.join(__dirname, '..', 'public', 'app.js'); + +function readAppSource() { + return fs.readFileSync(APP_PATH, 'utf8'); +} + +function skipString(src, i) { + const quote = src[i]; + i++; + while (i < src.length) { + const c = src[i]; + if (c === '\\') { i += 2; continue; } + if (quote === '`' && c === '$' && src[i + 1] === '{') { + i = skipBalanced(src, i + 1, '{', '}') + 1; + continue; + } + if (c === quote) return i; + i++; + } + throw new Error('unterminated string literal in app.js'); +} + +function skipBalanced(src, open, openCh, closeCh) { + let depth = 0; + let prev = ''; + for (let i = open; i < src.length; i++) { + const c = src[i]; + if (c === "'" || c === '"' || c === '`') { i = skipString(src, i); prev = c; continue; } + if (c === '/' && src[i + 1] === '/') { i = src.indexOf('\n', i); if (i === -1) break; continue; } + if (c === '/' && src[i + 1] === '*') { i = src.indexOf('*/', i) + 1; continue; } + if (c === '/' && /[(,=:[!&|?{};]/.test(prev)) { + i++; + while (src[i] !== '/') { if (src[i] === '\\') i++; i++; } + prev = '/'; + continue; + } + if (c === openCh) depth++; + else if (c === closeCh) { depth--; if (depth === 0) return i; } + if (!/\s/.test(c)) prev = c; + } + throw new Error(`unbalanced ${openCh}${closeCh} in app.js`); +} + +function extractFunction(src, name) { + const re = new RegExp(`^(async )?function ${name}\\(`, 'm'); + const m = re.exec(src); + if (!m) throw new Error(`public/app.js must define a top-level function ${name}`); + const paramsOpen = m.index + m[0].length - 1; + const paramsClose = skipBalanced(src, paramsOpen, '(', ')'); + const bodyOpen = src.indexOf('{', paramsClose); + const bodyClose = skipBalanced(src, bodyOpen, '{', '}'); + return src.slice(m.index, bodyClose + 1); +} + +function extractDeclaration(src, name) { + const re = new RegExp(`^(let|const) ${name}\\b[^\\n]*;[ \\t]*$`, 'm'); + const m = re.exec(src); + if (!m) throw new Error(`public/app.js must declare ${name} on a single line`); + return m[0]; +} + +// Evaluates the named declarations and functions of public/app.js in `context` +// (a jsdom VM context) and returns the functions by name. +function loadAppFunctions(context, { functions, declarations = [] }) { + const src = readAppSource(); + const parts = [ + ...declarations.map((n) => extractDeclaration(src, n)), + ...functions.map((n) => extractFunction(src, n)), + ]; + vm.runInContext(parts.join('\n'), context, { filename: APP_PATH }); + const out = {}; + for (const n of functions) out[n] = vm.runInContext(n, context); + return out; +} + +module.exports = { loadAppFunctions, extractFunction, extractDeclaration, readAppSource }; diff --git a/test/confirm-and-stop-session.test.js b/test/confirm-and-stop-session.test.js index 6bf8a1e8..5ce09e6d 100644 --- a/test/confirm-and-stop-session.test.js +++ b/test/confirm-and-stop-session.test.js @@ -3,170 +3,119 @@ // failure on the clicked button instead of alert(). See // .ai/contexts/session-state.md ("The two lifecycle verbs: detach and stop"). // -// app.js cannot be eval-ed in jsdom (module-scope `new ViewerPanel(...)` etc. -// — see test/running-indicators.test.js's file header for the full reason). -// `makeConfirmAndStopSession` below is therefore a HAND-MAINTAINED MIRROR of -// the real function, not the shipped code — it pins the *decision* logic in -// isolation. The source-level pin test at the bottom catches the one -// regression that matters most (the guard silently dropped from the shipped -// file) without needing a full eval — same two-layer technique already used -// in test/running-indicators.test.js. +// confirmAndStopSession is extracted from the real public/app.js +// (test/app-source.js) and runs in the jsdom window of dom-setup.js, next to +// the real resolveSessionStop of stop-session-ui.js. Only its outside edges +// (confirm, the IPC bridge, timers, the terminal view) are stubbed. 'use strict'; const test = require('node:test'); const assert = require('node:assert/strict'); -const fs = require('node:fs'); -const path = require('node:path'); - -const APP_SRC = fs.readFileSync(path.join(__dirname, '..', 'public', 'app.js'), 'utf8'); - -// Mirrors public/app.js's confirmAndStopSession(sessionId, btn), with every -// external dependency injected instead of read off globals. -function makeConfirmAndStopSession(deps) { - return async function confirmAndStopSession(sessionId, btn) { - const plan = deps.resolveSessionStop(deps.sessionMap.get(sessionId)); - if (!deps.confirm(plan.confirmText)) return; - const result = plan.remote - ? await deps.api.remoteStopSession(plan.alias, sessionId) - : await deps.api.stopSession(sessionId); - if (result && result.ok === false) { - const message = result.error || 'unknown error'; - deps.logError('[stop-session]', message); - if (btn) { - if (typeof deps.flashButtonText === 'function') deps.flashButtonText(btn, 'Failed', 1500); - const originalTitle = btn.title; - btn.title = message; - deps.setTimeout(() => { btn.title = originalTitle; }, 3000); - } - return; - } - if (plan.remote && typeof deps.applyRemoteStopped === 'function') { - deps.applyRemoteStopped(sessionId); - } - deps.activePtyIds.delete(sessionId); - if (!deps.gridViewActive && deps.activeSessionId === sessionId) { - deps.setActiveSession(null); - deps.closeTerminalView(); - } - }; +const { setupSidebarDom } = require('./dom-setup'); +const { loadAppFunctions } = require('./app-source'); + +function withHarness(overrides, fn) { + const ctx = setupSidebarDom(); + try { + const { window, document } = ctx; + const calls = { closed: [], active: [], refreshed: 0, logged: [], remoteStopped: [], flashed: [], ipc: [] }; + window.sessionMap = new Map([['s1', { sessionId: 's1', remoteAlias: 'vps' }]]); + window.activePtyIds = new Set(['s1']); + window.activeSessionId = 's1'; + window.gridViewActive = false; + window.terminalHeader = document.createElement('div'); + window.placeholder = document.createElement('div'); + window.setActiveSession = (id) => { calls.active.push(id); window.activeSessionId = id; }; + window.refreshSidebar = () => { calls.refreshed++; }; + window.applyRemoteStopped = (id) => calls.remoteStopped.push(id); + window.flashButtonText = (b, text, ms) => calls.flashed.push({ b, text, ms }); + window.confirm = () => true; + window.setTimeout = (cb) => cb(); + window.console = { error: (...args) => calls.logged.push(args) }; + window.api = { + stopSession: async (id) => { calls.ipc.push(['stop', id]); return { ok: true }; }, + remoteStopSession: async (alias, id) => { calls.ipc.push(['remote-stop', alias, id]); return { ok: true }; }, + }; + Object.assign(window, overrides(calls, window)); + const { confirmAndStopSession } = loadAppFunctions(ctx.context, { functions: ['confirmAndStopSession'] }); + return fn({ confirmAndStopSession, window, calls }); + } finally { + ctx.destroy(); + } } -function makeDeps(overrides = {}) { - const activePtyIds = new Set(['s1']); - return { - resolveSessionStop: (session) => (session && session.remoteAlias - ? { remote: true, alias: session.remoteAlias, confirmText: `Stop this session on ${session.remoteAlias}?` } - : { remote: false, alias: null, confirmText: 'Stop this session?' }), - sessionMap: new Map([['s1', { sessionId: 's1', remoteAlias: 'vps' }]]), - confirm: () => true, - api: { stopSession: async () => ({ ok: true }), remoteStopSession: async () => ({ ok: true }) }, - logError: () => {}, - flashButtonText: () => {}, - applyRemoteStopped: () => {}, - activePtyIds, - gridViewActive: false, - activeSessionId: 's1', - setActiveSession: () => {}, - closeTerminalView: () => {}, - setTimeout: (fn) => fn(), // fire immediately — tests don't care about the 3s delay itself - ...overrides, - }; -} +const none = () => ({}); test('a successful remote stop deletes the pty id and closes the active view', async () => { - const closed = []; - const deps = makeDeps({ - closeTerminalView: () => closed.push('closed'), + await withHarness(none, async ({ confirmAndStopSession, window, calls }) => { + await confirmAndStopSession('s1', null); + + assert.ok(!window.activePtyIds.has('s1'), 'activePtyIds must be cleared on success'); + assert.deepEqual(calls.active, [null], 'the active session must be released on success'); + assert.equal(window.terminalHeader.style.display, 'none', 'the terminal header must be hidden on success'); + assert.deepEqual(calls.remoteStopped, ['s1'], 'a remote stop must update the remote adapter state'); + assert.deepEqual(calls.ipc, [['remote-stop', 'vps', 's1']]); }); - const confirmAndStopSession = makeConfirmAndStopSession(deps); - - await confirmAndStopSession('s1', null); - - assert.ok(!deps.activePtyIds.has('s1'), 'activePtyIds must be cleared on success'); - assert.equal(closed.length, 1, 'the active terminal view must be closed on success'); }); test('a failed remote stop must not delete the pty id (mutation target: unconditional delete)', async () => { - const closed = []; - const deps = makeDeps({ - api: { stopSession: async () => ({ ok: true }), remoteStopSession: async () => ({ ok: false, error: 'pid now belongs to a non-claude process' }) }, - closeTerminalView: () => closed.push('closed'), + const failing = () => ({ + api: { + stopSession: async () => ({ ok: true }), + remoteStopSession: async () => ({ ok: false, error: 'pid now belongs to a non-claude process' }), + }, }); - const confirmAndStopSession = makeConfirmAndStopSession(deps); - - await confirmAndStopSession('s1', null); + await withHarness(failing, async ({ confirmAndStopSession, window, calls }) => { + await confirmAndStopSession('s1', null); - assert.ok(deps.activePtyIds.has('s1'), 'a failed stop must leave activePtyIds untouched'); - assert.equal(closed.length, 0, 'a failed stop must not close the terminal view'); + assert.ok(window.activePtyIds.has('s1'), 'a failed stop must leave activePtyIds untouched'); + assert.deepEqual(calls.active, [], 'a failed stop must not release the active session'); + assert.equal(window.terminalHeader.style.display, '', 'a failed stop must not hide the terminal header'); + assert.equal(calls.refreshed, 0); + }); }); test('a failed stop flashes the clicked button and sets its title to the error, restoring it after', async () => { - const flashCalls = []; - let restoredTitle = null; - const btn = { title: 'Stop session' }; - const deps = makeDeps({ + const titles = []; + const failing = () => ({ api: { remoteStopSession: async () => ({ ok: false, error: 'ssh: connection refused' }) }, - flashButtonText: (b, text, ms) => flashCalls.push({ b, text, ms }), - setTimeout: (fn) => { fn(); restoredTitle = btn.title; }, // simulate the restore firing later, capture it before we assert }); - const confirmAndStopSession = makeConfirmAndStopSession(deps); + await withHarness(failing, async ({ confirmAndStopSession, window, calls }) => { + const btn = { title: 'Stop session' }; + window.setTimeout = (cb) => { titles.push(btn.title); cb(); }; - await confirmAndStopSession('s1', btn); + await confirmAndStopSession('s1', btn); - assert.equal(flashCalls.length, 1, 'flashButtonText must be called on failure'); - assert.equal(flashCalls[0].text, 'Failed'); - // btn.title was set to the error message synchronously before the restore timer fires. - assert.equal(restoredTitle, 'Stop session', 'the original title is restored after the flash window'); + assert.equal(calls.flashed.length, 1, 'flashButtonText must be called on failure'); + assert.equal(calls.flashed[0].text, 'Failed'); + assert.deepEqual(titles, ['ssh: connection refused'], 'the error text is the button title until the timer fires'); + assert.equal(btn.title, 'Stop session', 'the original title is restored after the flash window'); + }); }); test('a failed stop with no button element does not throw (terminal header stop passes a real btn, but be defensive)', async () => { - const deps = makeDeps({ api: { remoteStopSession: async () => ({ ok: false, error: 'boom' }) } }); - const confirmAndStopSession = makeConfirmAndStopSession(deps); - await assert.doesNotReject(confirmAndStopSession('s1', null)); -}); - -test('declining the confirm() dialog calls neither IPC', async () => { - const calls = []; - const deps = makeDeps({ - confirm: () => false, - api: { - stopSession: async () => { calls.push('stop'); return { ok: true }; }, - remoteStopSession: async () => { calls.push('remote-stop'); return { ok: true }; }, - }, + const failing = () => ({ api: { remoteStopSession: async () => ({ ok: false, error: 'boom' }) } }); + await withHarness(failing, async ({ confirmAndStopSession }) => { + await assert.doesNotReject(confirmAndStopSession('s1', null)); }); - const confirmAndStopSession = makeConfirmAndStopSession(deps); - await confirmAndStopSession('s1', null); - assert.deepEqual(calls, []); }); -// --------------------------------------------------------------------------- -// Source-level pin for the REAL public/app.js. -// --------------------------------------------------------------------------- +test('a local session is stopped through stopSession, not the remote channel', async () => { + const local = () => ({ sessionMap: new Map([['s1', { sessionId: 's1' }]]) }); + await withHarness(local, async ({ confirmAndStopSession, calls }) => { + await confirmAndStopSession('s1', null); -test('public/app.js: confirmAndStopSession still exists with the (sessionId, btn) signature', () => { - assert.match(APP_SRC, /async function confirmAndStopSession\(sessionId,\s*btn\)/, - 'confirmAndStopSession must accept the clicked button as its second argument'); + assert.deepEqual(calls.ipc, [['stop', 's1']]); + assert.deepEqual(calls.remoteStopped, []); + }); }); -test('public/app.js: a failed stop returns before touching activePtyIds or closing the view (mutation target: unconditional delete)', () => { - const start = APP_SRC.indexOf('async function confirmAndStopSession(sessionId, btn)'); - assert.notEqual(start, -1); - const body = APP_SRC.slice(start, start + 1200); - - const failIdx = body.indexOf("result.ok === false"); - assert.notEqual(failIdx, -1, 'the ok === false branch must still exist'); - const returnIdx = body.indexOf('return;', failIdx); - const deleteIdx = body.indexOf('activePtyIds.delete(sessionId)'); - assert.notEqual(returnIdx, -1, 'the failure branch must return before falling through to the success path'); - assert.ok(returnIdx < deleteIdx, - 'the failure branch\'s return must precede activePtyIds.delete — a failed stop must not clear it'); -}); +test('declining the confirm() dialog calls neither IPC', async () => { + const declining = () => ({ confirm: () => false }); + await withHarness(declining, async ({ confirmAndStopSession, window, calls }) => { + await confirmAndStopSession('s1', null); -test('public/app.js: a failed stop flashes the button and sets its title to the error text', () => { - const start = APP_SRC.indexOf('async function confirmAndStopSession(sessionId, btn)'); - const body = APP_SRC.slice(start, start + 1200); - assert.match(body, /window\.flashButtonText\(btn,\s*'Failed',\s*1500\)/, - 'a failed stop must flash the button, same pattern as session-delete-btn'); - assert.match(body, /btn\.title\s*=\s*message/, 'the error text must be surfaced via the button title'); - assert.doesNotMatch(body, /\balert\(/, 'no alert() for a failed stop'); + assert.deepEqual(calls.ipc, []); + assert.ok(window.activePtyIds.has('s1')); + }); }); diff --git a/test/dom-setup.js b/test/dom-setup.js index 3f62ac28..31a9f276 100644 --- a/test/dom-setup.js +++ b/test/dom-setup.js @@ -155,6 +155,7 @@ function setupSidebarDom() { return { window, document: window.document, + context: ctx, sidebar: { renderProjects: window.renderProjects, buildSessionItem: window.buildSessionItem, diff --git a/test/indexing-banner.test.js b/test/indexing-banner.test.js index 0ccea77d..88eb780c 100644 --- a/test/indexing-banner.test.js +++ b/test/indexing-banner.test.js @@ -1,11 +1,8 @@ // Tests for the first-run cold-start indexing banner. // -// app.js is a monolithic renderer file that performs many document.getElementById -// calls and kicks off loadProjects() at module load time, making it impractical to -// load via vm.runInContext in jsdom without a massive DOM scaffolding. So the thin DOM-toggle wrapper -// (updateIndexingBanner) is exercised via a hand-wired harness that mirrors its body, -// while the actual text-formatting logic — formatIndexingBannerText, a pure function -// that lives in public/utils.js — is loaded and tested for real via dom-setup.js. +// updateIndexingBanner and dismissIndexingBanner are extracted from the real +// public/app.js (test/app-source.js) and run in the jsdom window that +// dom-setup.js builds, next to the real formatIndexingBannerText of utils.js. // // Invariants under test: // 1. formatIndexingBannerText renders "i/N projects, X sessions so far". @@ -24,11 +21,9 @@ const test = require('node:test'); const assert = require('node:assert/strict'); +const vm = require('node:vm'); const { setupSidebarDom } = require('./dom-setup'); - -// --------------------------------------------------------------------------- -// formatIndexingBannerText — real function, loaded via the shared jsdom harness. -// --------------------------------------------------------------------------- +const { loadAppFunctions } = require('./app-source'); test('formatIndexingBannerText: renders the one-time first-run message with counters', () => { const { window, destroy } = setupSidebarDom(); @@ -40,93 +35,82 @@ test('formatIndexingBannerText: renders the one-time first-run message with coun } }); -// --------------------------------------------------------------------------- -// updateIndexingBanner — hand-wired harness mirroring app.js's DOM-toggle logic. -// --------------------------------------------------------------------------- - -function makeHarness() { - const banner = { style: { display: 'none' } }; - const bannerText = { textContent: '' }; - let dismissed = false; - - // Reproduce the logic from app.js's updateIndexingBanner + dismiss handler. - // (formatIndexingBannerText itself is the real function, tested above; the - // harness inlines its two output shapes.) - function formatText(payload) { - if (payload.error) return `Indexing failed: ${payload.error} — it will resume on the next launch.`; - return `Indexing your Claude Code history — one-time, ${payload.current}/${payload.total} projects, ${payload.sessionsSoFar} sessions so far`; - } - function updateIndexingBanner(payload) { - if (!payload || !payload.coldStart) return; - if (payload.done) { - if (payload.error) { - bannerText.textContent = formatText(payload); - banner.style.display = ''; - dismissed = false; - return; - } - banner.style.display = 'none'; - dismissed = false; // a future cold-start run gets its own banner - return; - } - if (dismissed) return; - bannerText.textContent = formatText(payload); - banner.style.display = ''; - } - - function dismissIndexingBanner() { +function withHarness(fn) { + const ctx = setupSidebarDom(); + try { + const banner = ctx.document.createElement('div'); banner.style.display = 'none'; - dismissed = true; + const bannerText = ctx.document.createElement('span'); + banner.appendChild(bannerText); + ctx.window.indexingBanner = banner; + ctx.window.indexingBannerText = bannerText; + const fns = loadAppFunctions(ctx.context, { + declarations: ['indexingBannerDismissed'], + functions: ['updateIndexingBanner', 'dismissIndexingBanner'], + }); + fn({ + banner, + bannerText, + updateIndexingBanner: fns.updateIndexingBanner, + dismissIndexingBanner: fns.dismissIndexingBanner, + isDismissed: () => vm.runInContext('indexingBannerDismissed', ctx.context), + }); + } finally { + ctx.destroy(); } - - return { banner, bannerText, updateIndexingBanner, dismissIndexingBanner, isDismissed: () => dismissed }; } test('updateIndexingBanner: shows the banner with progress text on a cold-start event', () => { - const { banner, bannerText, updateIndexingBanner } = makeHarness(); - updateIndexingBanner({ coldStart: true, current: 1, total: 16, sessionsSoFar: 4, done: false }); - assert.equal(banner.style.display, ''); - assert.match(bannerText.textContent, /1\/16 projects, 4 sessions so far/); + withHarness(({ banner, bannerText, updateIndexingBanner }) => { + updateIndexingBanner({ coldStart: true, current: 1, total: 16, sessionsSoFar: 4, done: false }); + assert.equal(banner.style.display, ''); + assert.match(bannerText.textContent, /1\/16 projects, 4 sessions so far/); + }); }); test('updateIndexingBanner: hides the banner immediately on done:true', () => { - const { banner, updateIndexingBanner } = makeHarness(); - updateIndexingBanner({ coldStart: true, current: 5, total: 16, sessionsSoFar: 50, done: false }); - assert.equal(banner.style.display, ''); - updateIndexingBanner({ coldStart: true, current: 16, total: 16, sessionsSoFar: 200, done: true }); - assert.equal(banner.style.display, 'none', 'banner must disappear once indexing completes'); + withHarness(({ banner, updateIndexingBanner }) => { + updateIndexingBanner({ coldStart: true, current: 5, total: 16, sessionsSoFar: 50, done: false }); + assert.equal(banner.style.display, ''); + updateIndexingBanner({ coldStart: true, current: 16, total: 16, sessionsSoFar: 200, done: true }); + assert.equal(banner.style.display, 'none', 'banner must disappear once indexing completes'); + }); }); test('updateIndexingBanner: ignores events without coldStart (warm-start rebuilds never emit these, but defend the gate)', () => { - const { banner, updateIndexingBanner } = makeHarness(); - updateIndexingBanner({ coldStart: false, current: 1, total: 5, sessionsSoFar: 1, done: false }); - assert.equal(banner.style.display, 'none', 'no coldStart flag must never show the banner'); + withHarness(({ banner, updateIndexingBanner }) => { + updateIndexingBanner({ coldStart: false, current: 1, total: 5, sessionsSoFar: 1, done: false }); + assert.equal(banner.style.display, 'none', 'no coldStart flag must never show the banner'); + }); }); test('updateIndexingBanner: ignores a null/undefined payload without throwing', () => { - const { banner, updateIndexingBanner } = makeHarness(); - assert.doesNotThrow(() => updateIndexingBanner(null)); - assert.equal(banner.style.display, 'none'); + withHarness(({ banner, updateIndexingBanner }) => { + assert.doesNotThrow(() => updateIndexingBanner(null)); + assert.equal(banner.style.display, 'none'); + }); }); test('dismissIndexingBanner: hides the banner immediately, before done:true', () => { - const { banner, updateIndexingBanner, dismissIndexingBanner } = makeHarness(); - updateIndexingBanner({ coldStart: true, current: 2, total: 16, sessionsSoFar: 10, done: false }); - assert.equal(banner.style.display, '', 'sanity: banner is showing before dismiss'); + withHarness(({ banner, updateIndexingBanner, dismissIndexingBanner }) => { + updateIndexingBanner({ coldStart: true, current: 2, total: 16, sessionsSoFar: 10, done: false }); + assert.equal(banner.style.display, '', 'sanity: banner is showing before dismiss'); - dismissIndexingBanner(); + dismissIndexingBanner(); - assert.equal(banner.style.display, 'none', 'dismiss must hide the banner without waiting for done:true'); + assert.equal(banner.style.display, 'none', 'dismiss must hide the banner without waiting for done:true'); + }); }); test('dismissIndexingBanner: once dismissed, further non-done progress events do not re-show the banner', () => { - const { banner, updateIndexingBanner, dismissIndexingBanner } = makeHarness(); - updateIndexingBanner({ coldStart: true, current: 2, total: 16, sessionsSoFar: 10, done: false }); - dismissIndexingBanner(); + withHarness(({ banner, updateIndexingBanner, dismissIndexingBanner }) => { + updateIndexingBanner({ coldStart: true, current: 2, total: 16, sessionsSoFar: 10, done: false }); + dismissIndexingBanner(); - updateIndexingBanner({ coldStart: true, current: 3, total: 16, sessionsSoFar: 20, done: false }); + updateIndexingBanner({ coldStart: true, current: 3, total: 16, sessionsSoFar: 20, done: false }); - assert.equal(banner.style.display, 'none', 'a dismissed banner must stay hidden until the run completes'); + assert.equal(banner.style.display, 'none', 'a dismissed banner must stay hidden until the run completes'); + }); }); test('formatIndexingBannerText: a payload with an error renders the failure message', () => { @@ -140,34 +124,37 @@ test('formatIndexingBannerText: a payload with an error renders the failure mess }); test('updateIndexingBanner: done:true with an error shows the failure instead of hiding the banner', () => { - const { banner, bannerText, updateIndexingBanner } = makeHarness(); - updateIndexingBanner({ coldStart: true, current: 2, total: 16, sessionsSoFar: 10, done: false }); + withHarness(({ banner, bannerText, updateIndexingBanner }) => { + updateIndexingBanner({ coldStart: true, current: 2, total: 16, sessionsSoFar: 10, done: false }); - updateIndexingBanner({ coldStart: true, current: 5, total: 16, sessionsSoFar: 40, done: true, error: 'worker exited unexpectedly' }); + updateIndexingBanner({ coldStart: true, current: 5, total: 16, sessionsSoFar: 40, done: true, error: 'worker exited unexpectedly' }); - assert.equal(banner.style.display, '', 'a failed scan must stay visible, not silently disappear'); - assert.match(bannerText.textContent, /Indexing failed: worker exited unexpectedly/); + assert.equal(banner.style.display, '', 'a failed scan must stay visible, not silently disappear'); + assert.match(bannerText.textContent, /Indexing failed: worker exited unexpectedly/); + }); }); test('updateIndexingBanner: a failure surfaces even after the user dismissed the progress banner', () => { - const { banner, bannerText, updateIndexingBanner, dismissIndexingBanner } = makeHarness(); - updateIndexingBanner({ coldStart: true, current: 2, total: 16, sessionsSoFar: 10, done: false }); - dismissIndexingBanner(); + withHarness(({ banner, bannerText, updateIndexingBanner, dismissIndexingBanner }) => { + updateIndexingBanner({ coldStart: true, current: 2, total: 16, sessionsSoFar: 10, done: false }); + dismissIndexingBanner(); - updateIndexingBanner({ coldStart: true, current: 5, total: 16, sessionsSoFar: 40, done: true, error: 'boom' }); + updateIndexingBanner({ coldStart: true, current: 5, total: 16, sessionsSoFar: 40, done: true, error: 'boom' }); - assert.equal(banner.style.display, '', - '"your history did not finish indexing" is new information, not more of the dismissed progress stream'); - assert.match(bannerText.textContent, /Indexing failed: boom/); + assert.equal(banner.style.display, '', + '"your history did not finish indexing" is new information, not more of the dismissed progress stream'); + assert.match(bannerText.textContent, /Indexing failed: boom/); + }); }); test('a done:true event resets the dismissed flag for a future cold-start run', () => { - const { updateIndexingBanner, dismissIndexingBanner, isDismissed } = makeHarness(); - updateIndexingBanner({ coldStart: true, current: 2, total: 16, sessionsSoFar: 10, done: false }); - dismissIndexingBanner(); - assert.equal(isDismissed(), true); + withHarness(({ updateIndexingBanner, dismissIndexingBanner, isDismissed }) => { + updateIndexingBanner({ coldStart: true, current: 2, total: 16, sessionsSoFar: 10, done: false }); + dismissIndexingBanner(); + assert.equal(isDismissed(), true); - updateIndexingBanner({ coldStart: true, current: 16, total: 16, sessionsSoFar: 200, done: true }); + updateIndexingBanner({ coldStart: true, current: 16, total: 16, sessionsSoFar: 200, done: true }); - assert.equal(isDismissed(), false, 'done:true must clear the dismissed flag so a later run gets its own banner'); + assert.equal(isDismissed(), false, 'done:true must clear the dismissed flag so a later run gets its own banner'); + }); }); diff --git a/test/open-session-terminal.test.js b/test/open-session-terminal.test.js index 6aa152e9..169d9172 100644 --- a/test/open-session-terminal.test.js +++ b/test/open-session-terminal.test.js @@ -2,182 +2,153 @@ // open-terminal as a plain terminal. See .ai/contexts/session-state.md // ("Reopening a plain terminal"). // -// app.js cannot be eval-ed in jsdom (module-scope `new ViewerPanel(...)` etc. -// — see test/running-indicators.test.js's file header for the full reason). -// `makeOpenSession` below is therefore a HAND-MAINTAINED MIRROR of the real -// function, not the shipped code — it pins the *decision* logic in isolation. -// The source-level pins at the bottom catch the regressions that matter in the -// shipped file without needing a full eval — the same two-layer technique as -// test/confirm-and-stop-session.test.js. +// openSession is extracted from the real public/app.js (test/app-source.js) +// and runs in the jsdom window of dom-setup.js; the terminal, the IPC bridge +// and the other renderer files it calls into are stubbed. 'use strict'; const test = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); - -const APP_SRC = fs.readFileSync(path.join(__dirname, '..', 'public', 'app.js'), 'utf8'); - -// Mirrors public/app.js's openSession(session, customOptions), with every -// external dependency injected instead of read off globals. -function makeOpenSession(deps) { - return async function openSession(session, customOptions) { - const { sessionId, projectPath } = session; - - if (deps.openSessions.has(sessionId)) { - const entry = deps.openSessions.get(sessionId); - if (entry.closed) { - deps.destroySession(sessionId); - } else { - deps.showSession(sessionId); - return; - } - } - - const entry = deps.createTerminalEntry(session); - const resumeOptions = customOptions - || (session.type === 'terminal' ? { type: 'terminal' } : await deps.resolveDefaultSessionOptions({ projectPath })); - const result = await deps.api.openTerminal(sessionId, projectPath, false, resumeOptions, entry.initialSize); - if (!result.ok) { - deps.markFailed(sessionId, result.error); - return; - } - deps.showSession(sessionId); - }; -} - -function makeDeps(overrides = {}) { - const calls = { openTerminal: [], destroyed: [], shown: [], created: [], resolveDefaults: 0, launchTerminal: [] }; - const deps = { - calls, - openSessions: new Map(), - destroySession: (id) => calls.destroyed.push(id), - showSession: (id) => calls.shown.push(id), - createTerminalEntry: (session) => { calls.created.push(session.sessionId); return { initialSize: { cols: 80, rows: 24 } }; }, - resolveDefaultSessionOptions: async () => { calls.resolveDefaults++; return { permissionMode: 'plan' }; }, - // The mint-a-new-session path openSession must no longer take. - launchTerminalSession: (project) => calls.launchTerminal.push(project), - markFailed: () => {}, - api: { - openTerminal: async (sessionId, projectPath, isNew, sessionOptions, initialSize) => { - calls.openTerminal.push({ sessionId, projectPath, isNew, sessionOptions, initialSize }); - return { ok: true }; +const { setupSidebarDom } = require('./dom-setup'); +const { loadAppFunctions } = require('./app-source'); + +async function withHarness(setup, fn) { + const ctx = setupSidebarDom(); + try { + const { window } = ctx; + const calls = { openTerminal: [], destroyed: [], shown: [], created: [], resolveDefaults: 0, launchTerminal: [], written: [] }; + Object.assign(window, { + restoringWorkingSet: false, + sessionOpenedOutsideRestore: false, + openSessions: new Map(), + skippedWorkingSetEntries: new Set(), + destroySession: (id) => calls.destroyed.push(id), + showSession: (id) => calls.shown.push(id), + guardResume: async () => true, + createTerminalEntry: (session) => { + calls.created.push(session.sessionId); + return { initialSize: { cols: 80, rows: 24 }, terminal: { write: (text) => calls.written.push(text) } }; }, - }, - ...overrides, - }; - return deps; + resolveDefaultSessionOptions: async () => { calls.resolveDefaults++; return { permissionMode: 'plan' }; }, + launchTerminalSession: (project) => calls.launchTerminal.push(project), + forgetSessionExit: () => {}, + syncPtySizeAfterOpen: () => {}, + setSessionMcpActive: () => {}, + setSessionSandboxed: () => {}, + schedulePersistWorkingSet: () => {}, + pollActiveSessions: () => {}, + api: { + openTerminal: async (sessionId, projectPath, isNew, sessionOptions, initialSize) => { + calls.openTerminal.push({ sessionId, projectPath, isNew, sessionOptions, initialSize }); + return { ok: true }; + }, + }, + }); + setup(window, calls); + const { openSession } = loadAppFunctions(ctx.context, { functions: ['openSession'] }); + await fn({ openSession, window, calls }); + } finally { + ctx.destroy(); + } } +const noSetup = () => {}; +const plain = (value) => JSON.parse(JSON.stringify(value)); const TERMINAL_SESSION = { sessionId: 'term-uuid', projectPath: '/proj', type: 'terminal' }; const CLAUDE_SESSION = { sessionId: 'claude-uuid', projectPath: '/proj' }; // --- The intent that was being dropped ------------------------------------- test('a terminal session that is not open reaches open-terminal as a plain terminal (mutation target: the dropped type)', async () => { - const deps = makeDeps(); - await makeOpenSession(deps)(TERMINAL_SESSION); - - assert.equal(deps.calls.openTerminal.length, 1); - assert.deepEqual(deps.calls.openTerminal[0].sessionOptions, { type: 'terminal' }, - 'main.js reads sessionOptions.type === "terminal"; without it a shell id is handed to claude --resume'); - assert.equal(deps.calls.resolveDefaults, 0, - 'a shell has no permission mode, worktree or MCP emulation to resolve'); + await withHarness(noSetup, async ({ openSession, calls }) => { + await openSession(TERMINAL_SESSION); + + assert.equal(calls.openTerminal.length, 1); + assert.deepEqual(plain(calls.openTerminal[0].sessionOptions), { type: 'terminal' }, + 'main.js reads sessionOptions.type === "terminal"; without it a shell id is handed to claude --resume'); + assert.equal(calls.resolveDefaults, 0, + 'a shell has no permission mode, worktree or MCP emulation to resolve'); + }); }); test('a Claude session still resumes with the project\'s current defaults, and carries no terminal type', async () => { - const deps = makeDeps(); - await makeOpenSession(deps)(CLAUDE_SESSION); + await withHarness(noSetup, async ({ openSession, calls }) => { + await openSession(CLAUDE_SESSION); - assert.equal(deps.calls.resolveDefaults, 1); - assert.deepEqual(deps.calls.openTerminal[0].sessionOptions, { permissionMode: 'plan' }); - assert.equal(deps.calls.openTerminal[0].sessionOptions.type, undefined); + assert.equal(calls.resolveDefaults, 1); + assert.deepEqual(plain(calls.openTerminal[0].sessionOptions), { permissionMode: 'plan' }); + assert.equal(calls.openTerminal[0].sessionOptions.type, undefined); + }); }); test('an explicit customOptions still wins — the resume-with-config dialog is not overridden', async () => { - const deps = makeDeps(); - const chosen = { permissionMode: 'acceptEdits', chrome: true }; - await makeOpenSession(deps)(CLAUDE_SESSION, chosen); + await withHarness(noSetup, async ({ openSession, calls }) => { + const chosen = { permissionMode: 'acceptEdits', chrome: true }; + await openSession(CLAUDE_SESSION, chosen); - assert.equal(deps.calls.openTerminal[0].sessionOptions, chosen); - assert.equal(deps.calls.resolveDefaults, 0); + assert.equal(calls.openTerminal[0].sessionOptions, chosen); + assert.equal(calls.resolveDefaults, 0); + }); }); test('customOptions wins over the terminal default too, so the precedence has one rule, not two', async () => { - const deps = makeDeps(); - const chosen = { type: 'terminal', panelFor: 'owner' }; - await makeOpenSession(deps)(TERMINAL_SESSION, chosen); + await withHarness(noSetup, async ({ openSession, calls }) => { + const chosen = { type: 'terminal', panelFor: 'owner' }; + await openSession(TERMINAL_SESSION, chosen); - assert.equal(deps.calls.openTerminal[0].sessionOptions, chosen); + assert.equal(calls.openTerminal[0].sessionOptions, chosen); + }); }); // --- The orphaned row ------------------------------------------------------ test('a terminal whose shell exited reopens under its own id instead of minting a second row', async () => { - const deps = makeDeps(); - deps.openSessions.set('term-uuid', { closed: true }); - await makeOpenSession(deps)(TERMINAL_SESSION); - - assert.deepEqual(deps.calls.destroyed, ['term-uuid'], 'the dead entry is torn down first'); - assert.deepEqual(deps.calls.launchTerminal, [], - 'minting a new id leaves the clicked row pointing at an id nothing can open'); - assert.equal(deps.calls.openTerminal.length, 1); - assert.equal(deps.calls.openTerminal[0].sessionId, 'term-uuid', 'the row you clicked is the row that comes back'); - assert.deepEqual(deps.calls.openTerminal[0].sessionOptions, { type: 'terminal' }); + const setup = (window) => window.openSessions.set('term-uuid', { closed: true }); + await withHarness(setup, async ({ openSession, calls }) => { + await openSession(TERMINAL_SESSION); + + assert.deepEqual(calls.destroyed, ['term-uuid'], 'the dead entry is torn down first'); + assert.deepEqual(calls.launchTerminal, [], + 'minting a new id leaves the clicked row pointing at an id nothing can open'); + assert.equal(calls.openTerminal.length, 1); + assert.equal(calls.openTerminal[0].sessionId, 'term-uuid', 'the row you clicked is the row that comes back'); + assert.deepEqual(plain(calls.openTerminal[0].sessionOptions), { type: 'terminal' }); + }); }); test('an exited Claude session still reopens in place, the way it always did', async () => { - const deps = makeDeps(); - deps.openSessions.set('claude-uuid', { closed: true }); - await makeOpenSession(deps)(CLAUDE_SESSION); + const setup = (window) => window.openSessions.set('claude-uuid', { closed: true }); + await withHarness(setup, async ({ openSession, calls }) => { + await openSession(CLAUDE_SESSION); - assert.deepEqual(deps.calls.destroyed, ['claude-uuid']); - assert.equal(deps.calls.openTerminal[0].sessionId, 'claude-uuid'); + assert.deepEqual(calls.destroyed, ['claude-uuid']); + assert.equal(calls.openTerminal[0].sessionId, 'claude-uuid'); + }); }); test('a live entry is only shown — no second PTY for a terminal that is already running', async () => { - const deps = makeDeps(); - deps.openSessions.set('term-uuid', { closed: false }); - await makeOpenSession(deps)(TERMINAL_SESSION); - - assert.deepEqual(deps.calls.shown, ['term-uuid']); - assert.equal(deps.calls.openTerminal.length, 0); - assert.equal(deps.calls.created.length, 0); + const setup = (window) => window.openSessions.set('term-uuid', { closed: false }); + await withHarness(setup, async ({ openSession, calls }) => { + await openSession(TERMINAL_SESSION); + + assert.deepEqual(calls.shown, ['term-uuid']); + assert.equal(calls.openTerminal.length, 0); + assert.equal(calls.created.length, 0); + }); }); -// --------------------------------------------------------------------------- -// Source-level pins for the REAL public/app.js. -// --------------------------------------------------------------------------- - -function openSessionBody() { - const start = APP_SRC.indexOf('async function openSession(session, customOptions'); - assert.notEqual(start, -1, 'openSession must still exist with the (session, customOptions) signature'); - const end = APP_SRC.indexOf('\n}', APP_SRC.indexOf('pollActiveSessions();', start)); - assert.ok(end > start); - return APP_SRC.slice(start, end); -} - -test('public/app.js: openSession passes { type: \'terminal\' } for a terminal session (mutation target: reverting to the bare defaults call)', () => { - const body = openSessionBody(); - assert.match(body, /session\.type === 'terminal'\s*\?\s*\{\s*type:\s*'terminal'\s*\}/, - 'a terminal session must supply its own type rather than the Claude launch defaults'); -}); - -test('public/app.js: customOptions is still the first term of the resumeOptions chain', () => { - const body = openSessionBody(); - const chain = body.slice(body.indexOf('const resumeOptions')); - const custom = chain.indexOf('customOptions'); - const terminal = chain.indexOf("session.type === 'terminal'"); - assert.notEqual(custom, -1); - assert.ok(custom < terminal, 'an explicit choice from the resume dialog must keep winning'); -}); +test('a failed open-terminal marks the entry closed and writes the error into its terminal', async () => { + const setup = (window) => { + window.api.openTerminal = async () => ({ ok: false, error: 'spawn failed' }); + }; + await withHarness(setup, async ({ openSession, calls }) => { + await openSession(CLAUDE_SESSION); -test('public/app.js: openSession no longer mints a new session for an exited terminal', () => { - const body = openSessionBody(); - assert.doesNotMatch(body, /launchTerminalSession\(/, - 'minting a second id leaves the original sidebar row pointing at an id nothing can open'); - assert.match(body, /window\.api\.openTerminal\(sessionId,/, - 'the reopen must target the session id that was clicked'); + assert.match(calls.written.join(''), /Error: spawn failed/); + assert.deepEqual(calls.shown, ['claude-uuid'], 'the failed entry is still shown so the error is visible'); + }); }); test('public/dialogs.js: resolveDefaultSessionOptions still returns Claude launch options only', () => { diff --git a/test/running-indicators.test.js b/test/running-indicators.test.js index 50f0fbe4..7f09db0d 100644 --- a/test/running-indicators.test.js +++ b/test/running-indicators.test.js @@ -1,475 +1,244 @@ // Tests for Q9: updateRunningIndicators() pty-set gating. // -// app.js cannot be eval-ed in jsdom: module scope constructs real classes -// (`new ViewerPanel(...)`, line 25) and wires xterm/WebGL terminal setup -// across ~1500 LOC of renderer glue, none of which jsdom can stand in for -// without effectively re-building the whole public/ dependency chain -// (verified directly: eval-ing the real file throws `ViewerPanel is not -// defined` before it even reaches the code under test here). -// -// `makeIndicatorFn` below is therefore a HAND-MAINTAINED MIRROR of -// `updateRunningIndicators()`, not the shipped function — it tests the -// gating *logic* in isolation, not app.js's actual behavior. Keep it in sync -// by hand on every edit to the real function; a source-level test at the -// bottom of this file (`public/app.js: ...guard is still present`) catches -// the one regression that matters most (the guard being silently dropped -// from the shipped file) without needing a full eval. +// updateRunningIndicators is extracted from the real public/app.js +// (test/app-source.js) and runs in the jsdom window of dom-setup.js, next to +// the real session-activity.js / sidebar.js / remote-activity-ui.js it calls +// into. Only the grid card map, which grid-view.js owns, is supplied by the +// test. // // a) When activePtyIds is unchanged between calls, the two sidebar // querySelectorAll scans are skipped entirely. // b) When activePtyIds changes, the scans run and classes are updated. // c) The gridCards loop runs on every call (not gated) because sessionBusyState // can change independently of the pty-set. +// d) Subagent rows and remote rows are exempt from the PTY-set purge. 'use strict'; const test = require('node:test'); const assert = require('node:assert/strict'); -const fs = require('node:fs'); -const path = require('node:path'); -const { JSDOM } = require('jsdom'); const { setupSidebarDom } = require('./dom-setup'); - -// --------------------------------------------------------------------------- -// Minimal DOM setup -// --------------------------------------------------------------------------- - -function buildDom() { - const dom = new JSDOM(` - - `, { url: 'http://localhost/' }); - return dom; +const { loadAppFunctions } = require('./app-source'); + +function addSessionItem(ctx, sessionId, { remoteAlias, subagent, withGroup } = {}) { + const item = ctx.document.createElement('div'); + item.className = 'session-item'; + item.dataset.sessionId = sessionId; + if (remoteAlias) item.dataset.remoteAlias = remoteAlias; + if (subagent) item.dataset.subagent = '1'; + item.innerHTML = '
'; + const parent = withGroup || ctx.document.getElementById('sidebar-content'); + parent.appendChild(item); + return item; } -// Build the updateRunningIndicators function as it exists in public/app.js -// (post-Q9 patch). We instantiate it inline rather than eval-ing app.js because -// app.js registers IPC listeners at module-scope that require a preload bridge -// we can't stub cleanly in jsdom. -function makeIndicatorFn(doc, state) { - // Mirrors the module-level var added by Q9. - let lastPtySignature = ''; - - return function updateRunningIndicators() { - const sig = Array.from(state.activePtyIds).sort().join(','); - const ptySetChanged = sig !== lastPtySignature; - lastPtySignature = sig; - - if (ptySetChanged) { - doc.querySelectorAll('.session-item').forEach(item => { - // Subagents never own a PTY — their .running state is tracked - // separately via activeSubagentsByParent (sidebar.js), driven by the - // subagent-spawned/completed IPC pair, not activePtyIds (issue #129). - if (item.dataset.subagent) return; - const id = item.dataset.sessionId; - const running = state.activePtyIds.has(id); - item.classList.toggle('has-running-pty', running); - if (!running) { - item.classList.remove('needs-attention', 'response-ready', 'cli-busy', 'has-busy-agents'); - state.attentionSessions.delete(id); - state.responseReadySessions.delete(id); - state.sessionBusyState.delete(id); - // Mirrors app.js: a stopped PTY can never emit subagent-completed, - // so the live-subagent state is dropped immediately (sidebar.js's - // clearActiveSubagentsFor) instead of waiting for the 60s TTL. - if (state.clearActiveSubagentsFor) state.clearActiveSubagentsFor(id); - } - const icon = item.querySelector('.session-icon'); - if (icon) icon.classList.toggle('running', running); - }); - doc.querySelectorAll('.slug-group').forEach(group => { - const hasRunning = group.querySelector('.session-item.has-running-pty') !== null; - const dot = group.querySelector('.slug-group-dot'); - if (dot) dot.classList.toggle('running', hasRunning); - }); - } - - for (const [sid, card] of state.gridCards) { - const running = state.activePtyIds.has(sid); - const busy = state.sessionBusyState.get(sid) || false; - const dot = card.querySelector('.grid-card-dot'); - if (dot) dot.className = 'grid-card-dot ' + (busy ? 'busy' : (running ? 'running' : 'stopped')); - const footer = card.querySelector('.grid-card-footer'); - if (footer) footer.children[0].textContent = running ? 'Running' : 'Stopped'; - const stopBtn = card.querySelector('.grid-card-stop-btn'); - if (stopBtn) stopBtn.style.display = running ? '' : 'none'; - } - }; +function addSlugGroup(ctx) { + const group = ctx.document.createElement('div'); + group.className = 'slug-group'; + group.innerHTML = '
'; + ctx.document.getElementById('sidebar-content').appendChild(group); + return group; } -// --------------------------------------------------------------------------- -// Tests -// --------------------------------------------------------------------------- +function withIndicators(fn) { + const ctx = setupSidebarDom(); + try { + ctx.window.gridCards = new Map(); + const { updateRunningIndicators } = loadAppFunctions(ctx.context, { + declarations: ['_lastPtySignature'], + functions: ['updateRunningIndicators'], + }); + fn({ ctx, update: updateRunningIndicators }); + } finally { + ctx.destroy(); + } +} test('updateRunningIndicators: unchanged pty-set — sidebar querySelectorAll skipped', () => { - const dom = buildDom(); - const { window } = dom; - const { document } = window; - - // Spy on querySelectorAll to count sidebar scans. - let qsaCallCount = 0; - const origQsa = document.querySelectorAll.bind(document); - document.querySelectorAll = (...args) => { - // Only count the selector patterns updateRunningIndicators uses for sidebar - // scans; not the internal DOM reads like slug-group-dot lookups (which are - // called on elements, not document). - if (args[0] === '.session-item' || args[0] === '.slug-group') qsaCallCount++; - return origQsa(...args); - }; - - const state = { - activePtyIds: new Set(['s1']), - attentionSessions: new Set(), - responseReadySessions: new Set(), - sessionBusyState: new Map(), - gridCards: new Map(), - }; - const update = makeIndicatorFn(document, state); - - // First call — pty-set changed from '' → 's1'; sidebar scan MUST run. - update(); - assert.equal(qsaCallCount, 2, 'first call: both .session-item and .slug-group scanned'); - const item1 = document.querySelector('[data-session-id="s1"]'); - assert.ok(item1.classList.contains('has-running-pty'), 's1 has-running-pty set on first call'); - - // Second call — same activePtyIds; sidebar scan must be SKIPPED. - qsaCallCount = 0; - update(); - assert.equal(qsaCallCount, 0, 'second call with same pty-set: sidebar querySelectorAll NOT called'); - - window.close(); + withIndicators(({ ctx, update }) => { + const group = addSlugGroup(ctx); + const item1 = addSessionItem(ctx, 's1', { withGroup: group }); + addSessionItem(ctx, 's2', { withGroup: group }); + ctx.window.activePtyIds = new Set(['s1']); + + let qsaCallCount = 0; + const origQsa = ctx.document.querySelectorAll.bind(ctx.document); + ctx.document.querySelectorAll = (...args) => { + if (args[0] === '.session-item' || args[0] === '.slug-group') qsaCallCount++; + return origQsa(...args); + }; + + update(); + assert.equal(qsaCallCount, 2, 'first call: both .session-item and .slug-group scanned'); + assert.ok(item1.classList.contains('has-running-pty'), 's1 has-running-pty set on first call'); + + qsaCallCount = 0; + update(); + assert.equal(qsaCallCount, 0, 'second call with same pty-set: sidebar querySelectorAll NOT called'); + }); }); test('updateRunningIndicators: changed pty-set — sidebar scans run, classes updated', () => { - const dom = buildDom(); - const { window } = dom; - const { document } = window; - - const state = { - activePtyIds: new Set(['s1']), - attentionSessions: new Set(), - responseReadySessions: new Set(), - sessionBusyState: new Map(), - gridCards: new Map(), - }; - const update = makeIndicatorFn(document, state); - - // First call: s1 running. - update(); - const item1 = document.querySelector('[data-session-id="s1"]'); - const item2 = document.querySelector('[data-session-id="s2"]'); - assert.ok(item1.classList.contains('has-running-pty'), 's1 running after first call'); - assert.ok(!item2.classList.contains('has-running-pty'), 's2 not running'); - - // Change the pty-set: now s2 running, s1 stopped. - state.activePtyIds = new Set(['s2']); - update(); - assert.ok(!item1.classList.contains('has-running-pty'), 's1 no longer running after set change'); - assert.ok(item2.classList.contains('has-running-pty'), 's2 now running'); - - // Slug group dot should reflect at least one running session. - const groupDot = document.querySelector('.slug-group-dot'); - assert.ok(groupDot.classList.contains('running'), 'slug-group-dot running when s2 is running'); - - window.close(); + withIndicators(({ ctx, update }) => { + const group = addSlugGroup(ctx); + const item1 = addSessionItem(ctx, 's1', { withGroup: group }); + const item2 = addSessionItem(ctx, 's2', { withGroup: group }); + ctx.window.activePtyIds = new Set(['s1']); + + update(); + assert.ok(item1.classList.contains('has-running-pty'), 's1 running after first call'); + assert.ok(!item2.classList.contains('has-running-pty'), 's2 not running'); + + ctx.window.activePtyIds = new Set(['s2']); + update(); + assert.ok(!item1.classList.contains('has-running-pty'), 's1 no longer running after set change'); + assert.ok(item2.classList.contains('has-running-pty'), 's2 now running'); + + const groupDot = group.querySelector('.slug-group-dot'); + assert.ok(groupDot.classList.contains('running'), 'slug-group-dot running when s2 is running'); + + ctx.window.activePtyIds = new Set(); + update(); + assert.ok(!groupDot.classList.contains('running'), 'slug-group-dot cleared once nothing runs'); + }); }); test('updateRunningIndicators: stale attention/response-ready/cli-busy cleared when pty stops', () => { - const dom = buildDom(); - const { window } = dom; - const { document } = window; - - const attentionSessions = new Set(['s1']); - const responseReadySessions = new Set(['s1']); - const sessionBusyState = new Map([['s1', true]]); - const clearedSubagentParents = []; - const state = { - activePtyIds: new Set(['s1']), - attentionSessions, - responseReadySessions, - sessionBusyState, - gridCards: new Map(), - clearActiveSubagentsFor: (id) => clearedSubagentParents.push(id), - }; - const update = makeIndicatorFn(document, state); - - // First call: s1 running — no cleanup. - update(); - assert.ok(state.attentionSessions.has('s1'), 's1 attention preserved while running'); - assert.ok(!clearedSubagentParents.includes('s1'), 'subagent state untouched while s1 runs'); - - // s1 stops. - state.activePtyIds = new Set(); - const item1 = document.querySelector('[data-session-id="s1"]'); - item1.classList.add('needs-attention', 'response-ready', 'cli-busy', 'has-busy-agents'); - update(); - - assert.ok(!state.attentionSessions.has('s1'), 'attentionSessions cleared when pty stops'); - assert.ok(!state.responseReadySessions.has('s1'), 'responseReadySessions cleared'); - assert.ok(!state.sessionBusyState.has('s1'), 'sessionBusyState cleared'); - assert.ok(!item1.classList.contains('needs-attention'), '.needs-attention removed'); - assert.ok(!item1.classList.contains('response-ready'), '.response-ready removed'); - assert.ok(!item1.classList.contains('cli-busy'), '.cli-busy removed'); - assert.ok(!item1.classList.contains('has-busy-agents'), '.has-busy-agents removed — a killed PTY never emits subagent-completed'); - assert.ok(clearedSubagentParents.includes('s1'), 'clearActiveSubagentsFor called so the sidebar state cannot resurrect the indicator'); - - window.close(); + withIndicators(({ ctx, update }) => { + const item1 = addSessionItem(ctx, 's1'); + const clearedSubagentParents = []; + const realClear = ctx.window.clearActiveSubagentsFor; + ctx.window.clearActiveSubagentsFor = (id) => { clearedSubagentParents.push(id); return realClear(id); }; + ctx.window.activePtyIds = new Set(['s1']); + + ctx.setActivity('s1', true, 'onCliBusyState'); + ctx.attentionSessions.add('s1'); + ctx.responseReadySessions.add('s1'); + update(); + assert.ok(ctx.attentionSessions.has('s1'), 's1 attention preserved while running'); + assert.deepEqual(clearedSubagentParents, [], 'subagent state untouched while s1 runs'); + + ctx.window.activePtyIds = new Set(); + item1.classList.add('needs-attention', 'response-ready', 'has-busy-agents'); + update(); + + assert.ok(!ctx.attentionSessions.has('s1'), 'attentionSessions cleared when pty stops'); + assert.ok(!ctx.responseReadySessions.has('s1'), 'responseReadySessions cleared'); + assert.ok(!ctx.sessionBusyState.has('s1'), 'sessionBusyState cleared'); + assert.ok(!item1.classList.contains('cli-busy'), '.cli-busy removed'); + assert.ok(!item1.classList.contains('needs-attention'), '.needs-attention removed'); + assert.ok(!item1.classList.contains('response-ready'), '.response-ready removed'); + assert.ok(!item1.classList.contains('has-busy-agents'), '.has-busy-agents removed — a killed PTY never emits subagent-completed'); + assert.deepEqual(clearedSubagentParents, ['s1'], 'clearActiveSubagentsFor called so the sidebar state cannot resurrect the indicator'); + }); }); test('updateRunningIndicators: gridCards loop runs on every call, not gated by pty-set', () => { - const dom = buildDom(); - const { window } = dom; - const { document } = window; - - // Build a fake grid card with the expected structure. - const makeCard = (doc) => { - const card = doc.createElement('div'); + withIndicators(({ ctx, update }) => { + const card = ctx.document.createElement('div'); card.innerHTML = `
`; - return card; - }; - - const card = makeCard(document); - const state = { - activePtyIds: new Set(['s1']), - attentionSessions: new Set(), - responseReadySessions: new Set(), - sessionBusyState: new Map([['s1', false]]), - gridCards: new Map([['s1', card]]), - }; - const update = makeIndicatorFn(document, state); - - // First call: s1 running, not busy. - update(); - assert.equal(card.querySelector('.grid-card-dot').className, 'grid-card-dot running', - 'grid card dot is running'); - assert.equal(card.querySelector('.grid-card-footer').children[0].textContent, 'Running', - 'grid card footer shows Running'); - assert.equal(card.querySelector('.grid-card-stop-btn').style.display, '', - 'stop button visible'); - - // Second call: pty-set UNCHANGED, but sessionBusyState changed to busy. - // The gridCards loop must still run (not gated). - state.sessionBusyState.set('s1', true); - update(); - assert.equal(card.querySelector('.grid-card-dot').className, 'grid-card-dot busy', - 'grid card dot updated to busy on second call even though pty-set unchanged'); - - window.close(); + ctx.window.gridCards = new Map([['s1', card]]); + ctx.window.activePtyIds = new Set(['s1']); + ctx.sessionBusyState.set('s1', false); + + update(); + assert.equal(card.querySelector('.grid-card-dot').className, 'grid-card-dot running', 'grid card dot is running'); + assert.equal(card.querySelector('.grid-card-footer').children[0].textContent, 'Running', 'grid card footer shows Running'); + assert.equal(card.querySelector('.grid-card-stop-btn').style.display, '', 'stop button visible'); + + ctx.sessionBusyState.set('s1', true); + update(); + assert.equal(card.querySelector('.grid-card-dot').className, 'grid-card-dot busy', + 'grid card dot updated to busy on second call even though pty-set unchanged'); + + ctx.window.activePtyIds = new Set(); + ctx.sessionBusyState.set('s1', false); + update(); + assert.equal(card.querySelector('.grid-card-dot').className, 'grid-card-dot stopped'); + assert.equal(card.querySelector('.grid-card-stop-btn').style.display, 'none', 'stop button hidden once stopped'); + }); }); -test('makeIndicatorFn replica: subagent items (dataset.subagent) are untouched by the pty-set scan (issue #129)', () => { - // Documents the intended behavior of the hand-maintained replica above — - // this does NOT exercise public/app.js's real updateRunningIndicators (see - // file header: app.js can't be eval-ed in jsdom). Disabling the replica's - // own guard (line ~58) turns this red; disabling the *real* guard in - // public/app.js does not touch this test at all — that gap is covered - // separately by the source-level test at the bottom of this file. - // - // Without the guard, this scan runs `activePtyIds.has(id)` for the - // subagent's own sessionId — always false, since subagents never own a - // PTY — and would immediately clear the .running class + dot that - // sidebar.js's subagent-spawned listener just set. - const dom = buildDom(); - const { window } = dom; - const { document } = window; - - const sidebarContent = document.getElementById('sidebar-content'); - const subagentItem = document.createElement('div'); - subagentItem.className = 'session-item running'; - subagentItem.dataset.sessionId = 'sub:s-top-1:agent-1'; - subagentItem.dataset.subagent = '1'; - subagentItem.innerHTML = '
'; - sidebarContent.appendChild(subagentItem); - - const state = { - activePtyIds: new Set(['s1']), - attentionSessions: new Set(), - responseReadySessions: new Set(), - sessionBusyState: new Map(), - gridCards: new Map(), - }; - const update = makeIndicatorFn(document, state); - update(); // primes lastPtySignature - - // Change the pty-set (unrelated to the subagent) so the sidebar scan runs again. - state.activePtyIds = new Set(['s2']); - update(); - - assert.ok(subagentItem.classList.contains('running'), 'subagent item keeps .running across an unrelated pty-set change'); - assert.ok(subagentItem.querySelector('.session-icon').classList.contains('running'), 'subagent icon slot keeps .running'); - assert.ok(!subagentItem.classList.contains('has-running-pty'), 'subagent item never gets has-running-pty (no PTY, guard short-circuits before that toggle)'); - - window.close(); +test('updateRunningIndicators: subagent items (dataset.subagent) are untouched by the pty-set scan (issue #129)', () => { + // Without the guard, the scan runs `activePtyIds.has(id)` for the subagent's + // own sessionId — always false, since subagents never own a PTY — and would + // immediately clear the .running class that sidebar.js's subagent-spawned + // listener just set. + withIndicators(({ ctx, update }) => { + const subagentItem = addSessionItem(ctx, 'sub:s-top-1:agent-1', { subagent: true }); + subagentItem.classList.add('running'); + subagentItem.querySelector('.session-icon').classList.add('running'); + ctx.window.activePtyIds = new Set(['s1']); + update(); + + ctx.window.activePtyIds = new Set(['s2']); + update(); + + assert.ok(subagentItem.classList.contains('running'), 'subagent item keeps .running across an unrelated pty-set change'); + assert.ok(subagentItem.querySelector('.session-icon').classList.contains('running'), 'subagent icon slot keeps .running'); + assert.ok(!subagentItem.classList.contains('has-running-pty'), 'subagent item never gets has-running-pty'); + }); }); test('updateRunningIndicators: empty pty-set — all sessions marked stopped', () => { - const dom = buildDom(); - const { window } = dom; - const { document } = window; - - const state = { - activePtyIds: new Set(), - attentionSessions: new Set(), - responseReadySessions: new Set(), - sessionBusyState: new Map(), - gridCards: new Map(), - }; - const update = makeIndicatorFn(document, state); - - // Prime: first call with empty set. - update(); - - const items = document.querySelectorAll('.session-item'); - for (const item of items) { - assert.ok(!item.classList.contains('has-running-pty'), - `${item.dataset.sessionId} must not have has-running-pty when idle`); - } - - window.close(); + withIndicators(({ ctx, update }) => { + const items = [addSessionItem(ctx, 's1'), addSessionItem(ctx, 's2')]; + ctx.window.activePtyIds = new Set(['s1', 's2']); + update(); + assert.ok(items.every((item) => item.classList.contains('has-running-pty')), 'precondition: both rows running'); + + ctx.window.activePtyIds = new Set(); + update(); + + for (const item of items) { + assert.ok(!item.classList.contains('has-running-pty'), + `${item.dataset.sessionId} must not have has-running-pty when idle`); + } + }); }); // --------------------------------------------------------------------------- // F7 — remote rows are exempt from the PTY-set purge. -// -// Unlike the replica tests above (hand-rolled Maps/Sets), these drive the -// REAL session-activity.js/sidebar.js/remote-activity-ui.js via dom-setup.js -// (same technique as test/dom-sidebar-remote-session.test.js). The gating -// loop itself is still a hand-mirror of updateRunningIndicators — app.js -// cannot be eval'd in jsdom (see file header) — but the state it mutates -// (sessionBusyState/responseReadySessions/attentionSessions, and the purge -// itself) is the real purgeActivityFor from session-activity.js, not a -// replica. Keep this mirror's remote-skip condition in sync with app.js's -// `if (!running && !item.dataset.remoteAlias)`. // --------------------------------------------------------------------------- -function runIndicatorPass(doc, activePtyIds) { - doc.querySelectorAll('.session-item').forEach(item => { - if (item.dataset.subagent) return; - const id = item.dataset.sessionId; - const running = activePtyIds.has(id); - item.classList.toggle('has-running-pty', running); - if (!running && !item.dataset.remoteAlias) { - item.classList.remove('has-busy-agents'); - doc.defaultView.purgeActivityFor(id, 'pty-gone'); - } - }); -} - test('F7: a remote row busy via onRemoteActivityEvent stays .cli-busy across an unrelated local pty-set change', () => { - const ctx = setupSidebarDom(); - try { - const sidebarContent = ctx.document.getElementById('sidebar-content'); - const localItem = ctx.document.createElement('div'); - localItem.className = 'session-item'; - localItem.dataset.sessionId = 'local-1'; - localItem.innerHTML = ''; - const remoteItem = ctx.document.createElement('div'); - remoteItem.className = 'session-item'; - remoteItem.dataset.sessionId = 'remote-1'; - remoteItem.dataset.remoteAlias = 'vps'; - remoteItem.innerHTML = ''; - sidebarContent.append(localItem, remoteItem); - - // Real onRemoteActivityEvent (remote-activity-ui.js), as the watch - // channel's IPC event would drive it — routes through the real setActivity. + withIndicators(({ ctx, update }) => { + const localItem = addSessionItem(ctx, 'local-1'); + const remoteItem = addSessionItem(ctx, 'remote-1', { remoteAlias: 'vps' }); + ctx.window.onRemoteActivityEvent({ sessionId: 'remote-1' }); assert.ok(remoteItem.classList.contains('cli-busy'), 'precondition: remote row busy via the real dispatcher'); assert.equal(ctx.sessionBusyState.get('remote-1'), true); - runIndicatorPass(ctx.document, new Set(['local-1'])); + ctx.window.activePtyIds = new Set(['local-1']); + update(); assert.ok(remoteItem.classList.contains('cli-busy'), 'remote row still busy after a pass with local-1 running'); - // local-1 stops — an unrelated local pty-set change. - runIndicatorPass(ctx.document, new Set()); + ctx.window.activePtyIds = new Set(); + update(); assert.ok(remoteItem.classList.contains('cli-busy'), 'remote row must NOT be purged by an unrelated local pty-set change'); assert.equal(ctx.sessionBusyState.get('remote-1'), true, 'sessionBusyState for the remote row is untouched'); assert.ok(!localItem.classList.contains('has-running-pty'), 'the local row is still correctly marked not running'); - } finally { - ctx.destroy(); - } + }); }); test('F7: a local non-running row is still purged through purgeActivityFor', () => { - const ctx = setupSidebarDom(); - try { - const sidebarContent = ctx.document.getElementById('sidebar-content'); - const localItem = ctx.document.createElement('div'); - localItem.className = 'session-item'; - localItem.dataset.sessionId = 'local-1'; - localItem.innerHTML = ''; - sidebarContent.append(localItem); + withIndicators(({ ctx, update }) => { + const localItem = addSessionItem(ctx, 'local-1'); ctx.setActivity('local-1', true, 'onCliBusyState'); - runIndicatorPass(ctx.document, new Set(['local-1'])); + ctx.window.activePtyIds = new Set(['local-1']); + update(); assert.ok(localItem.classList.contains('cli-busy'), 'precondition: local row busy while its pty runs'); - runIndicatorPass(ctx.document, new Set()); // the pty stops + ctx.window.activePtyIds = new Set(); + update(); assert.ok(!localItem.classList.contains('cli-busy'), 'a stopped local row must still be purged'); assert.equal(ctx.sessionBusyState.has('local-1'), false, 'sessionBusyState entry dropped for the stopped local row'); - } finally { - ctx.destroy(); - } -}); - -// --------------------------------------------------------------------------- -// Source-level pin for the REAL public/app.js (not the replica above). -// -// The replica tests above cannot detect a real-file regression: app.js can't -// be eval-ed in jsdom (see file header), so nothing here actually calls the -// shipped updateRunningIndicators. This test reads public/app.js's own -// source and asserts the subagent guard is still the first statement inside -// the `.session-item` forEach — the exact line whose removal would let the -// periodic pty-set poll wipe a subagent's .running indicator (issue #129). -// Removing that line from public/app.js turns this test red on its own, -// independent of the replica staying in sync. -// --------------------------------------------------------------------------- - -test('public/app.js: updateRunningIndicators still guards subagent items before touching activePtyIds', () => { - const src = fs.readFileSync(path.join(__dirname, '..', 'public', 'app.js'), 'utf8'); - const scanStart = src.indexOf("document.querySelectorAll('.session-item').forEach(item => {"); - assert.notEqual(scanStart, -1, 'the .session-item pty-set scan must still exist in public/app.js'); - - // The guard must appear before the forEach body reads item.dataset.sessionId - // — i.e. before any activePtyIds lookup for this item. - const body = src.slice(scanStart, scanStart + 800); - const guardIdx = body.search(/if\s*\(\s*item\.dataset\.subagent\s*\)\s*return;/); - const sessionIdIdx = body.indexOf('item.dataset.sessionId'); - - assert.notEqual(guardIdx, -1, 'public/app.js must still contain `if (item.dataset.subagent) return;` in the pty-set scan'); - assert.ok(guardIdx < sessionIdIdx, 'the subagent guard must run before the item is treated as a PTY-backed session'); -}); - -test('public/app.js: pty-stop cleanup removes has-busy-agents and purges the sidebar subagent state', () => { - // Same source-level pin technique as above (app.js cannot be eval-ed in - // jsdom). stop-session kills the PTY without a subagent-completed event and - // detectSubagentTransitions skips exited sessions, so this cleanup is the - // only thing standing between a stopped session and a ghost violet glyph - // that lingers until the 60s TTL prune. - const src = fs.readFileSync(path.join(__dirname, '..', 'public', 'app.js'), 'utf8'); - const scanStart = src.indexOf("document.querySelectorAll('.session-item').forEach(item => {"); - assert.notEqual(scanStart, -1, 'the .session-item pty-set scan must still exist in public/app.js'); - - const body = src.slice(scanStart, scanStart + 1200); - // Was a literal classList.remove('has-busy-agents', ...) before the DOM - // split in .ai/contexts/session-state.md — now routed through the - // projection file's setHasBusyAgents() (public/session-activity-dom.js), - // the only place allowed to touch this class (eslint.config.js). - assert.match(body, /setHasBusyAgents\(item,\s*false\)/, - "the !running cleanup must clear 'has-busy-agents' along with the other per-session state classes"); - assert.match(body, /clearActiveSubagentsFor\(id\)/, - 'the !running cleanup must purge activeSubagentsByParent via clearActiveSubagentsFor so a re-render cannot resurrect the indicator'); + }); }); diff --git a/test/search-perf.test.js b/test/search-perf.test.js index b1dcf58e..8a839806 100644 --- a/test/search-perf.test.js +++ b/test/search-perf.test.js @@ -10,106 +10,59 @@ // so clearing with resort:false would sort the full list against a stale index // and produce a scrambled sidebar order. // -// app.js cannot be eval-ed in jsdom (it registers IPC listeners at module -// scope before stubs are ready). We follow the running-indicators.test.js -// pattern: replicate the relevant logic inline and test it in isolation. +// clearSearch, resetSearchFilter and runSearchQuery are extracted from the real +// public/app.js (test/app-source.js) and run in the jsdom window of +// dom-setup.js. The sidebar refresh, the memory/work-file renderers, the IPC +// search and the animation-frame queue are stubbed so the tests can observe them. 'use strict'; const test = require('node:test'); const assert = require('node:assert/strict'); - -// --------------------------------------------------------------------------- -// Minimal in-process replica of the search functions from public/app.js. -// We keep it as close to the real source as possible so a drift in the real -// file shows up as a test failure on the next `task check`. -// --------------------------------------------------------------------------- - -const MIN_SEARCH_CHARS = 3; - -function makeSearchState() { - const state = { - activeTab: 'sessions', - searchMatchIds: null, - searchMatchProjectPaths: null, - cachedAllProjects: [], - searchTitlesOnly: false, - // Spies - refreshSidebarCalls: [], - renderMemoriesCalls: [], - renderWorkFilesCalls: [], - apiSearchCalls: [], - }; - - // Fake DOM handles - const inputEl = { value: '', _cleared: false }; - const searchBarEl = { classList: { removed: [], toggled: [] } }; - let debounceTimer = null; - - function refreshSidebar(opts) { - state.refreshSidebarCalls.push(opts); - } - function renderMemories(ids) { - state.renderMemoriesCalls.push(ids); - } - function renderWorkFiles(ids) { - state.renderWorkFilesCalls.push(ids); - } - - // Mirrors clearSearch() in app.js. - function clearSearch() { - inputEl.value = ''; - if (debounceTimer) { clearTimeout(debounceTimer); debounceTimer = null; } - if (state.activeTab === 'sessions') { - state.searchMatchIds = null; - state.searchMatchProjectPaths = null; - refreshSidebar({ resort: true }); // resort:true required — sortedOrder is stale after search - } else if (state.activeTab === 'memory') { - renderMemories(); - } else if (state.activeTab === 'work-files') { - renderWorkFiles(); - } - } - - // Mirrors resetSearchFilter() in app.js. - function resetSearchFilter() { - if (state.activeTab === 'sessions') { - state.searchMatchIds = null; - state.searchMatchProjectPaths = null; - refreshSidebar({ resort: true }); // resort:true required — same stale-sortedOrder reason - } else if (state.activeTab === 'memory') { - renderMemories(); - } else if (state.activeTab === 'work-files') { - renderWorkFiles(); - } - } - - // Mirrors runSearchQuery() in app.js. - async function runSearchQuery(apiSearch) { - const query = inputEl.value.trim(); - if (!query) { - clearSearch(); - return; - } - if (query.length < MIN_SEARCH_CHARS) { - resetSearchFilter(); - return; - } - // Would call window.api.search in real code: - state.apiSearchCalls.push({ query, tab: state.activeTab }); - await apiSearch(state.activeTab, query, state.searchTitlesOnly); - if (state.activeTab === 'sessions') { - state.searchMatchIds = new Set(['fake-result']); - refreshSidebar({ resort: true }); - } +const { setupSidebarDom } = require('./dom-setup'); +const { loadAppFunctions } = require('./app-source'); + +async function withSearch(fn) { + const ctx = setupSidebarDom(); + try { + const { window, document } = ctx; + const state = { + refreshSidebarCalls: [], + renderMemoriesCalls: [], + renderWorkFilesCalls: [], + apiSearchCalls: [], + apiResults: [], + frames: new Map(), + nextFrame: 1, + }; + const inputEl = document.createElement('input'); + window.searchInput = inputEl; + window.searchBar = document.createElement('div'); + window.activeTab = 'sessions'; + window.searchTitlesOnly = false; + window.refreshSidebar = (opts) => state.refreshSidebarCalls.push(opts); + window.renderMemories = (ids) => state.renderMemoriesCalls.push(ids); + window.renderWorkFiles = (ids) => state.renderWorkFilesCalls.push(ids); + window.requestAnimationFrame = (cb) => { const id = state.nextFrame++; state.frames.set(id, cb); return id; }; + window.cancelAnimationFrame = (id) => { state.frames.delete(id); }; + window.api = { + search: async (kind, query, titlesOnly) => { + state.apiSearchCalls.push({ kind, query, titlesOnly }); + return state.apiResults; + }, + }; + const flushFrames = () => { + const pending = [...state.frames.values()]; + state.frames.clear(); + for (const cb of pending) cb(); + }; + const fns = loadAppFunctions(ctx.context, { + declarations: ['MIN_SEARCH_CHARS', 'searchDebounceTimer', 'clearRenderRaf'], + functions: ['clearSearch', 'resetSearchFilter', 'runSearchQuery'], + }); + await fn({ ...fns, state, inputEl, window, flushFrames }); + } finally { + ctx.destroy(); } - - return { - state, - inputEl, - clearSearch, - resetSearchFilter, - runSearchQuery, - }; } // --------------------------------------------------------------------------- @@ -117,75 +70,89 @@ function makeSearchState() { // --------------------------------------------------------------------------- test('search: 2-char query does NOT call api.search and does NOT clear input', async () => { - const { state, inputEl, runSearchQuery } = makeSearchState(); - inputEl.value = 'ab'; - let apiCalled = false; - await runSearchQuery(() => { apiCalled = true; }); + await withSearch(async ({ state, inputEl, runSearchQuery }) => { + inputEl.value = 'ab'; + await runSearchQuery(); - assert.equal(apiCalled, false, 'api.search must not be called for a 2-char query'); - assert.equal(inputEl.value, 'ab', 'input value must be preserved (not cleared)'); + assert.equal(state.apiSearchCalls.length, 0, 'api.search must not be called for a 2-char query'); + assert.equal(inputEl.value, 'ab', 'input value must be preserved (not cleared)'); + }); }); test('search: 2-char query resets filter state (searchMatchIds = null)', async () => { - const { state, inputEl, runSearchQuery } = makeSearchState(); - // Simulate a prior active search - state.searchMatchIds = new Set(['old-session']); - inputEl.value = 'ab'; - await runSearchQuery(() => {}); - - assert.equal(state.searchMatchIds, null, 'searchMatchIds must be reset to null'); - assert.equal(state.searchMatchProjectPaths, null, 'searchMatchProjectPaths must be reset to null'); + await withSearch(async ({ inputEl, window, runSearchQuery }) => { + window.searchMatchIds = new Set(['old-session']); + window.searchMatchProjectPaths = new Set(['/old']); + inputEl.value = 'ab'; + await runSearchQuery(); + + assert.equal(window.searchMatchIds, null, 'searchMatchIds must be reset to null'); + assert.equal(window.searchMatchProjectPaths, null, 'searchMatchProjectPaths must be reset to null'); + }); }); test('search: 2-char query calls refreshSidebar (to show unfiltered list)', async () => { - const { state, inputEl, runSearchQuery } = makeSearchState(); - inputEl.value = 'ab'; - await runSearchQuery(() => {}); + await withSearch(async ({ state, inputEl, runSearchQuery, flushFrames }) => { + inputEl.value = 'ab'; + await runSearchQuery(); + flushFrames(); - assert.equal(state.refreshSidebarCalls.length, 1, 'refreshSidebar must be called once'); + assert.equal(state.refreshSidebarCalls.length, 1, 'refreshSidebar must be called once'); + }); }); test('search: 1-char query behaves the same as 2-char (below threshold)', async () => { - const { state, inputEl, runSearchQuery } = makeSearchState(); - inputEl.value = 'a'; - let apiCalled = false; - await runSearchQuery(() => { apiCalled = true; }); + await withSearch(async ({ state, inputEl, runSearchQuery }) => { + inputEl.value = 'a'; + await runSearchQuery(); - assert.equal(apiCalled, false, 'api.search must not be called for a 1-char query'); - assert.equal(inputEl.value, 'a', 'input value must be preserved'); + assert.equal(state.apiSearchCalls.length, 0, 'api.search must not be called for a 1-char query'); + assert.equal(inputEl.value, 'a', 'input value must be preserved'); + }); }); test('search: " ab " (2 trimmed chars) does NOT call api.search', async () => { - const { state, inputEl, runSearchQuery } = makeSearchState(); - inputEl.value = ' ab '; - let apiCalled = false; - await runSearchQuery(() => { apiCalled = true; }); - - assert.equal(apiCalled, false, 'trim semantics: 2 trimmed chars must not trigger search'); - // Input text must be preserved - assert.equal(inputEl.value, ' ab ', 'input value must be preserved'); + await withSearch(async ({ state, inputEl, runSearchQuery }) => { + inputEl.value = ' ab '; + await runSearchQuery(); + + assert.equal(state.apiSearchCalls.length, 0, 'trim semantics: 2 trimmed chars must not trigger search'); + assert.equal(inputEl.value, ' ab ', 'input value must be preserved'); + }); }); test('search: 3-char query DOES call api.search', async () => { - const { state, inputEl, runSearchQuery } = makeSearchState(); - inputEl.value = 'abc'; - let apiCalled = false; - await runSearchQuery(() => { apiCalled = true; return Promise.resolve([]); }); - - assert.equal(apiCalled, true, 'api.search must be called for a 3-char query'); - assert.equal(state.apiSearchCalls.length, 1); - assert.equal(state.apiSearchCalls[0].query, 'abc'); + await withSearch(async ({ state, inputEl, runSearchQuery }) => { + inputEl.value = 'abc'; + await runSearchQuery(); + + assert.equal(state.apiSearchCalls.length, 1); + assert.equal(state.apiSearchCalls[0].query, 'abc'); + assert.equal(state.apiSearchCalls[0].kind, 'session'); + }); }); test('search: empty query calls clearSearch (wipes input value)', async () => { - const { state, inputEl, runSearchQuery } = makeSearchState(); - inputEl.value = ''; - await runSearchQuery(() => {}); - - // clearSearch sets inputEl.value = '' - assert.equal(inputEl.value, '', 'empty query triggers full clearSearch'); - // refreshSidebar called once by clearSearch - assert.equal(state.refreshSidebarCalls.length, 1); + await withSearch(async ({ state, inputEl, runSearchQuery, flushFrames }) => { + inputEl.value = ''; + await runSearchQuery(); + flushFrames(); + + assert.equal(inputEl.value, '', 'empty query triggers full clearSearch'); + assert.equal(state.refreshSidebarCalls.length, 1); + assert.equal(state.apiSearchCalls.length, 0); + }); +}); + +test('search: whitespace-only query is treated as empty', async () => { + await withSearch(async ({ state, inputEl, runSearchQuery, flushFrames }) => { + inputEl.value = ' '; + await runSearchQuery(); + flushFrames(); + + assert.equal(inputEl.value, '', 'a blank query is cleared like an empty one'); + assert.equal(state.apiSearchCalls.length, 0); + }); }); // --------------------------------------------------------------------------- @@ -195,76 +162,82 @@ test('search: empty query calls clearSearch (wipes input value)', async () => { // the full project list order) // --------------------------------------------------------------------------- -test('clearSearch: calls refreshSidebar with resort:true (not resort:false)', () => { - const { state, clearSearch } = makeSearchState(); - state.searchMatchIds = new Set(['s1', 's2']); - clearSearch(); +test('clearSearch: calls refreshSidebar with resort:true (not resort:false)', async () => { + await withSearch(async ({ state, window, clearSearch, flushFrames }) => { + window.searchMatchIds = new Set(['s1', 's2']); + clearSearch(); + flushFrames(); - assert.equal(state.refreshSidebarCalls.length, 1, 'refreshSidebar called exactly once on clear'); - assert.equal(state.refreshSidebarCalls[0].resort, true, - 'clearSearch must pass resort:true — sortedOrder is stale after a search'); + assert.equal(state.refreshSidebarCalls.length, 1, 'refreshSidebar called exactly once on clear'); + assert.equal(state.refreshSidebarCalls[0].resort, true, + 'clearSearch must pass resort:true — sortedOrder is stale after a search'); + }); }); -test('clearSearch: resets searchMatchIds and searchMatchProjectPaths', () => { - const { state, clearSearch } = makeSearchState(); - state.searchMatchIds = new Set(['s1']); - state.searchMatchProjectPaths = new Set(['/home/dev/proj']); - clearSearch(); +test('clearSearch: resets searchMatchIds and searchMatchProjectPaths', async () => { + await withSearch(async ({ window, clearSearch }) => { + window.searchMatchIds = new Set(['s1']); + window.searchMatchProjectPaths = new Set(['/home/dev/proj']); + clearSearch(); - assert.equal(state.searchMatchIds, null); - assert.equal(state.searchMatchProjectPaths, null); + assert.equal(window.searchMatchIds, null); + assert.equal(window.searchMatchProjectPaths, null); + }); }); -test('clearSearch: does NOT call refreshSidebar more than once (no double-render)', () => { - const { state, clearSearch } = makeSearchState(); - clearSearch(); - assert.equal(state.refreshSidebarCalls.length, 1, - 'clearSearch must trigger exactly one refreshSidebar call'); +test('clearSearch: defers the rebuild to an animation frame, and rapid clears queue only one', async () => { + await withSearch(async ({ state, clearSearch, flushFrames }) => { + clearSearch(); + clearSearch(); + assert.equal(state.refreshSidebarCalls.length, 0, 'the heavy rebuild must wait for the next frame'); + + flushFrames(); + + assert.equal(state.refreshSidebarCalls.length, 1, 'two clears before a frame fires must rebuild once'); + }); }); test('resetSearchFilter: calls refreshSidebar with resort:true (not resort:false)', async () => { // Sequence: user types 3+ chars (search runs, searchMatchIds populated), // then deletes back to 2 chars — resetSearchFilter must re-sort from data // because sortedOrder was overwritten to contain only the matched subset. - const { state, resetSearchFilter } = makeSearchState(); - // Simulate a prior active search having populated searchMatchIds - state.searchMatchIds = new Set(['session-x']); - resetSearchFilter(); - - assert.equal(state.refreshSidebarCalls.length, 1, 'refreshSidebar called once'); - assert.equal(state.refreshSidebarCalls[0].resort, true, - 'resetSearchFilter must pass resort:true — sortedOrder is stale after prior search'); + await withSearch(async ({ state, window, resetSearchFilter, flushFrames }) => { + window.searchMatchIds = new Set(['session-x']); + resetSearchFilter(); + flushFrames(); + + assert.equal(state.refreshSidebarCalls.length, 1, 'refreshSidebar called once'); + assert.equal(state.refreshSidebarCalls[0].resort, true, + 'resetSearchFilter must pass resort:true — sortedOrder is stale after prior search'); + }); }); test('search: delete from 3+ chars to 2 chars resets filter (sequence scenario)', async () => { - // Simulates the sequence: type "abc" → results → delete to "ab" → unfiltered. - const { state, inputEl, runSearchQuery } = makeSearchState(); - // Step 1: 3-char search runs - inputEl.value = 'abc'; - await runSearchQuery(() => Promise.resolve([])); - assert.notEqual(state.searchMatchIds, null, 'search must have set searchMatchIds'); - - // Step 2: user deletes to 2 chars — triggers resetSearchFilter path - inputEl.value = 'ab'; - await runSearchQuery(() => {}); - - assert.equal(state.searchMatchIds, null, 'searchMatchIds must be cleared after drop to 2 chars'); - assert.equal(state.searchMatchProjectPaths, null, 'searchMatchProjectPaths must be cleared'); - // refreshSidebar must have been called for both the search and the reset - assert.ok(state.refreshSidebarCalls.length >= 2, 'refreshSidebar called for search and for reset'); - // The reset call must use resort:true - const resetCall = state.refreshSidebarCalls[state.refreshSidebarCalls.length - 1]; - assert.equal(resetCall.resort, true, 'reset call must use resort:true'); + await withSearch(async ({ state, inputEl, window, runSearchQuery, flushFrames }) => { + state.apiResults = [{ id: 'hit-1' }]; + inputEl.value = 'abc'; + await runSearchQuery(); + assert.deepEqual([...window.searchMatchIds], ['hit-1'], 'search must have set searchMatchIds'); + + inputEl.value = 'ab'; + await runSearchQuery(); + flushFrames(); + + assert.equal(window.searchMatchIds, null, 'searchMatchIds must be cleared after drop to 2 chars'); + assert.equal(window.searchMatchProjectPaths, null, 'searchMatchProjectPaths must be cleared'); + assert.equal(state.refreshSidebarCalls.length, 2, 'refreshSidebar called for the search and for the reset'); + assert.equal(state.refreshSidebarCalls[1].resort, true, 'reset call must use resort:true'); + }); }); test('search: " a " (1 trimmed char) does NOT call api.search and preserves input', async () => { - const { inputEl, runSearchQuery } = makeSearchState(); - inputEl.value = ' a '; - let apiCalled = false; - await runSearchQuery(() => { apiCalled = true; }); + await withSearch(async ({ state, inputEl, runSearchQuery }) => { + inputEl.value = ' a '; + await runSearchQuery(); - assert.equal(apiCalled, false, 'trim semantics: 1 trimmed char must not trigger search'); - assert.equal(inputEl.value, ' a ', 'input value must be preserved (not cleared)'); + assert.equal(state.apiSearchCalls.length, 0, 'trim semantics: 1 trimmed char must not trigger search'); + assert.equal(inputEl.value, ' a ', 'input value must be preserved (not cleared)'); + }); }); // --------------------------------------------------------------------------- @@ -272,12 +245,24 @@ test('search: " a " (1 trimmed char) does NOT call api.search and preserves in // --------------------------------------------------------------------------- test('search: 2-char query on memory tab calls renderMemories (not api.search)', async () => { - const { state, inputEl, runSearchQuery } = makeSearchState(); - state.activeTab = 'memory'; - inputEl.value = 'me'; - let apiCalled = false; - await runSearchQuery(() => { apiCalled = true; }); - - assert.equal(apiCalled, false, 'api.search not called for 2-char on memory tab'); - assert.equal(state.renderMemoriesCalls.length, 1, 'renderMemories called to show unfiltered list'); + await withSearch(async ({ state, inputEl, window, runSearchQuery }) => { + window.activeTab = 'memory'; + inputEl.value = 'me'; + await runSearchQuery(); + + assert.equal(state.apiSearchCalls.length, 0, 'api.search not called for 2-char on memory tab'); + assert.equal(state.renderMemoriesCalls.length, 1, 'renderMemories called to show unfiltered list'); + }); +}); + +test('search: 3-char query on the work-files tab searches work-file and renders the matches', async () => { + await withSearch(async ({ state, inputEl, window, runSearchQuery }) => { + window.activeTab = 'work-files'; + state.apiResults = [{ id: 'wf-1' }]; + inputEl.value = 'plan'; + await runSearchQuery(); + + assert.equal(state.apiSearchCalls[0].kind, 'work-file'); + assert.deepEqual([...state.renderWorkFilesCalls[0]], ['wf-1']); + }); }); From d2096a833233d9b01be9eef1e76a57ca8df8f901 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Date: Thu, 1 Oct 2026 22:31:53 +0200 Subject: [PATCH 2/2] (test): make the app.js function extractor refuse what it cannot parse The regex-literal skipper had no end-of-input guard and ignored [...] classes and keyword-led regexes. It now throws on an unterminated regex, and every extracted slice is compiled and checked to end at a line break, so a scanner mistake fails loudly naming the function instead of loading a truncated one. Unit tests cover the synthetic cases. Refs #171 --- test/app-source.js | 32 +++++++++++++++++++++++---- test/app-source.test.js | 48 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 4 deletions(-) create mode 100644 test/app-source.test.js diff --git a/test/app-source.js b/test/app-source.js index da3a73c5..35aaf2a3 100644 --- a/test/app-source.js +++ b/test/app-source.js @@ -31,6 +31,21 @@ function skipString(src, i) { throw new Error('unterminated string literal in app.js'); } +function skipRegex(src, start) { + let i = start + 1; + let inClass = false; + while (i < src.length) { + const c = src[i]; + if (c === '\n') break; + if (c === '\\') { i += 2; continue; } + if (c === '[') inClass = true; + else if (c === ']') inClass = false; + else if (c === '/' && !inClass) return i; + i++; + } + throw new Error('unterminated regular expression literal in app.js'); +} + function skipBalanced(src, open, openCh, closeCh) { let depth = 0; let prev = ''; @@ -39,9 +54,8 @@ function skipBalanced(src, open, openCh, closeCh) { if (c === "'" || c === '"' || c === '`') { i = skipString(src, i); prev = c; continue; } if (c === '/' && src[i + 1] === '/') { i = src.indexOf('\n', i); if (i === -1) break; continue; } if (c === '/' && src[i + 1] === '*') { i = src.indexOf('*/', i) + 1; continue; } - if (c === '/' && /[(,=:[!&|?{};]/.test(prev)) { - i++; - while (src[i] !== '/') { if (src[i] === '\\') i++; i++; } + if (c === '/' && (/[(,=:[!&|?{};]/.test(prev) || /\b(?:return|typeof|case|throw|void|delete|in|of)\s*$/.test(src.slice(Math.max(0, i - 10), i)))) { + i = skipRegex(src, i); prev = '/'; continue; } @@ -60,7 +74,17 @@ function extractFunction(src, name) { const paramsClose = skipBalanced(src, paramsOpen, '(', ')'); const bodyOpen = src.indexOf('{', paramsClose); const bodyClose = skipBalanced(src, bodyOpen, '{', '}'); - return src.slice(m.index, bodyClose + 1); + const slice = src.slice(m.index, bodyClose + 1); + const next = src[bodyClose + 1]; + if (next !== undefined && next !== '\n' && next !== '\r') { + throw new Error(`extraction of ${name} from app.js ended mid-line; the brace scanner lost track`); + } + try { + new vm.Script(slice); + } catch (err) { + throw new Error(`extraction of ${name} from app.js does not compile: ${err.message}`); + } + return slice; } function extractDeclaration(src, name) { diff --git a/test/app-source.test.js b/test/app-source.test.js new file mode 100644 index 00000000..401f9768 --- /dev/null +++ b/test/app-source.test.js @@ -0,0 +1,48 @@ +'use strict'; +const test = require('node:test'); +const assert = require('node:assert/strict'); +const { extractFunction, extractDeclaration } = require('./app-source'); + +test('extractFunction: braces inside strings, templates, comments and regexes do not end the body', () => { + const src = [ + 'function a(x = { k: 1 }) {', + " const s = '}' + \"}\" + `${ {a: 1}.a }}`; // }", + ' /* } */', + ' return /}/.test(s);', + '}', + 'function b() {}', + ].join('\n'); + assert.equal(extractFunction(src, 'a'), src.split('\nfunction b')[0]); +}); + +test('extractFunction: a slash inside a regex class does not end the regex', () => { + const src = 'function a() {\n return /[/]}/.test("x");\n}\nfunction b() {}'; + assert.equal(extractFunction(src, 'a'), 'function a() {\n return /[/]}/.test("x");\n}'); +}); + +test('extractFunction: a regex the scanner mistakes for a division is refused, not silently truncated', () => { + const src = 'function a(x) {\n if (x) /}/.test("y");\n return 1;\n}\n'; + assert.throws(() => extractFunction(src, 'a'), /extraction of a /); +}); + +test('extractFunction: an unterminated regex throws instead of looping', () => { + assert.throws(() => extractFunction('function a() {\n return /abc;\n}\n', 'a'), /unterminated regular expression/); +}); + +test('extractFunction: a body that does not close throws', () => { + assert.throws(() => extractFunction('function a() {\n return 1;\n', 'a'), /unbalanced/); +}); + +test('extractFunction: a slice that ends mid-line is refused and names the function', () => { + assert.throws(() => extractFunction('function a() { return 1; } trailing();\n', 'a'), /extraction of a .*mid-line/); +}); + +test('extractFunction: a missing function is reported by name', () => { + assert.throws(() => extractFunction('function a() {}\n', 'zzz'), /zzz/); +}); + +test('extractDeclaration: takes the single-line declaration, refuses a missing one', () => { + const src = "let x = 1;\nconst MIN = 3;\n"; + assert.equal(extractDeclaration(src, 'MIN'), 'const MIN = 3;'); + assert.throws(() => extractDeclaration(src, 'nope'), /nope/); +});