From 7beec7194a316f29310aa57fde5032826e8b3f05 Mon Sep 17 00:00:00 2001 From: arkon Date: Tue, 12 May 2026 09:57:08 +0200 Subject: [PATCH] fix(client): harden inline rename against CJK, mid-rename deletion, and double-fire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-up fixes to the inline rename input introduced in #81: 1. IME composition guard. Pressing Enter to confirm a Chinese pinyin candidate (or any IME composition) was committing the half-composed text as the session name. Skip the keydown handler when isComposing is true or when keyCode is the legacy 229 sentinel that older Safari/Edge versions report on the Enter that triggers compositionend. 2. Ghost tab on mid-rename deletion. If a session was deleted via SSE while its tab was being renamed, the render-skip flag suppressed _renderSessionTabs() and the orphaned stayed on screen until blur — at which point the rename PUT 404'd against the dead session. Replace the boolean _inlineRenameActive with a _activeRename {sessionId, cancel} object so _cleanupSessionData can abort an in-flight rename targeting the deleted session, and finishRename skips the API call when the session is gone. 3. Stuck-flag risk. Move the settle-once guard into a closure-local `settled` boolean so blur / Enter / Escape / external cancel all converge to a single idempotent path. Register _activeRename only after the input is fully wired so a throw earlier in setup can't strand state. Adds test/inline-rename.test.ts with 7 Playwright tests that drive startInlineRename via page.evaluate() against a stubbed session and synthetic .tab-name node — no real PTY/tmux needed, runs in ~1.3s. Also fixes test/mobile/helpers/server.ts which imported the WebServer via a path one directory short of the repo root, breaking the entire mobile test suite under the main vitest config. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/web/public/app.js | 9 +- src/web/public/session-ui.js | 44 ++++-- test/inline-rename.test.ts | 281 ++++++++++++++++++++++++++++++++++ test/mobile/helpers/server.ts | 2 +- 4 files changed, 320 insertions(+), 16 deletions(-) create mode 100644 test/inline-rename.test.ts diff --git a/src/web/public/app.js b/src/web/public/app.js index b4ebccd7..d26e68ae 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -1845,7 +1845,7 @@ class CodemanApp { renderSessionTabs() { // Don't re-render while user is typing in the inline rename input - if (this._inlineRenameActive) return; + if (this._activeRename) return; this._debouncedCall('sessionTabs', this._renderSessionTabsImmediate); } @@ -1990,7 +1990,7 @@ class CodemanApp { } _fullRenderSessionTabs() { - if (this._inlineRenameActive) return; + if (this._activeRename) return; const container = this.$('sessionTabs'); // Clean up any orphaned dropdowns before re-rendering @@ -2694,6 +2694,11 @@ class CodemanApp { // Shared cleanup for all session data — called from both closeSession() and session:deleted handler _cleanupSessionData(sessionId) { + // If the deleted session is currently being renamed, abort the rename + // so the inline doesn't ghost as a stale tab on screen. + if (this._activeRename?.sessionId === sessionId) { + this._activeRename.cancel(); + } this.sessions.delete(sessionId); // Remove from tab order const orderIndex = this.sessionOrder.indexOf(sessionId); diff --git a/src/web/public/session-ui.js b/src/web/public/session-ui.js index e66b4417..e32c1e07 100644 --- a/src/web/public/session-ui.js +++ b/src/web/public/session-ui.js @@ -912,8 +912,9 @@ Object.assign(CodemanApp.prototype, { const tabName = document.querySelector(`.tab-name[data-session-id="${sessionId}"]`); if (!tabName) return; - // Prevent tab re-renders from destroying the input while renaming - this._inlineRenameActive = true; + // If a previous rename somehow leaked (shouldn't happen, but defends against + // future code paths that throw before cleanup), abort it before starting fresh. + if (this._activeRename) this._activeRename.cancel(); const currentName = this.getSessionName(session); const parsed = parseSessionPrefix(session.name); @@ -941,19 +942,26 @@ Object.assign(CodemanApp.prototype, { input.focus(); input.select(); - const finishRename = async () => { - if (!this._inlineRenameActive) return; // prevent double-fire - this._inlineRenameActive = false; - const suffix = input.value.trim(); - let fullName; - if (parsed) { - fullName = parsed.prefix + (suffix ? ': ' + suffix : ''); - } else { - fullName = suffix; + let settled = false; + const finishRename = async ({ commit }) => { + if (settled) return; + settled = true; + this._activeRename = null; + + // Aborted (e.g. session was deleted mid-rename): just re-render so any + // ghost DOM left behind is replaced with the canonical tab list. + if (!commit) { + this.renderSessionTabs(); + return; } + + const suffix = input.value.trim(); + const fullName = parsed ? parsed.prefix + (suffix ? ': ' + suffix : '') : suffix; tabName.textContent = fullName || originalContent; - if (fullName !== session.name) { + // Skip the API call if the session vanished between focus and blur. + const stillExists = this.sessions.has(sessionId); + if (stillExists && fullName !== session.name) { try { await fetch(`/api/sessions/${sessionId}/name`, { method: 'PUT', @@ -969,8 +977,18 @@ Object.assign(CodemanApp.prototype, { this.renderSessionTabs(); }; - input.addEventListener('blur', finishRename); + // Register only after the input is wired so a throw above can't strand state. + this._activeRename = { + sessionId, + cancel: () => finishRename({ commit: false }), + }; + + input.addEventListener('blur', () => finishRename({ commit: true })); input.addEventListener('keydown', (e) => { + // Enter/Escape during IME composition belong to the IME (e.g. confirming + // a Chinese pinyin candidate). keyCode 229 is the legacy signal for the + // same condition on browsers that don't set isComposing reliably. + if (e.isComposing || e.keyCode === 229) return; if (e.key === 'Enter') { e.preventDefault(); input.blur(); diff --git a/test/inline-rename.test.ts b/test/inline-rename.test.ts new file mode 100644 index 00000000..df19daf0 --- /dev/null +++ b/test/inline-rename.test.ts @@ -0,0 +1,281 @@ +/** + * Inline rename input tests. + * + * Covers the three fixes shipped after the audit of #81: + * 1. CJK composition guard — Enter/Escape during IME composition belong to + * the IME and must not commit/cancel the rename. + * 2. Ghost tab cleanup — when a session is deleted while its tab is being + * renamed, _cleanupSessionData() must cancel the rename so the inline + * doesn't ghost on screen. + * 3. Settle-once — cancel()/blur convergence is idempotent and reliably + * clears _activeRename, even on repeated invocation. + * + * Strategy: stub a synthetic .tab-name node and a fake session entry, then + * drive the rename function directly via page.evaluate(). No real PTY/tmux. + * + * Port: 3164 (per MEMORY.md, ports 3150+ for tests) + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { chromium, type Browser, type Page } from 'playwright'; +import { WebServer } from '../src/web/server.js'; + +const PORT = 3164; +const BASE_URL = `http://localhost:${PORT}`; + +describe('Inline rename input', () => { + let server: WebServer; + let browser: Browser; + let page: Page; + + beforeAll(async () => { + server = new WebServer(PORT, false, true); // testMode = true + await server.start(); + browser = await chromium.launch({ headless: true }); + page = await browser.newPage(); + await page.goto(BASE_URL, { waitUntil: 'domcontentloaded' }); + // Wait for app.js to expose window.app and finish constructor init. + await page.waitForFunction( + () => + typeof (window as { app?: unknown }).app !== 'undefined' && + !!(window as { app?: { sessions?: Map } }).app?.sessions + ); + }, 60000); + + afterAll(async () => { + if (browser) await browser.close(); + if (server) await server.stop(); + }, 60000); + + // Reset state between tests so each starts from a clean slate. + async function resetState(): Promise { + await page.evaluate(() => { + const app = ( + window as unknown as { app: { _activeRename: { cancel: () => void } | null; sessions: Map } } + ).app; + if (app._activeRename) app._activeRename.cancel(); + app.sessions.clear(); + document.querySelectorAll('[data-test-tab]').forEach((n) => n.remove()); + }); + // Allow any cancel-triggered renderSessionTabs to settle. + await page.waitForTimeout(20); + } + + // Helper: stub a session + tab-name DOM node, then start rename. + // Returns whether the rename input was successfully created. + async function startRename(sessionId: string, name: string): Promise { + return page.evaluate( + ({ id, name }) => { + const app = ( + window as unknown as { + app: { + sessions: Map; + startInlineRename: (id: string) => void; + }; + } + ).app; + app.sessions.set(id, { id, name }); + const wrap = document.createElement('div'); + wrap.setAttribute('data-test-tab', '1'); + const tabName = document.createElement('span'); + tabName.className = 'tab-name'; + tabName.setAttribute('data-session-id', id); + tabName.textContent = name; + wrap.appendChild(tabName); + document.body.appendChild(wrap); + app.startInlineRename(id); + return !!tabName.querySelector('input.tab-rename-input'); + }, + { id: sessionId, name } + ); + } + + it('CJK guard: Enter with isComposing=true does not commit', async () => { + await resetState(); + expect(await startRename('cjk-isc', 'OldName')).toBe(true); + + const result = await page.evaluate(() => { + const app = (window as unknown as { app: { _activeRename: unknown } }).app; + const input = document.querySelector('input.tab-rename-input') as HTMLInputElement; + input.value = 'partial-pinyin'; + input.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter', isComposing: true, bubbles: true })); + return { + inputStillInDom: document.body.contains(input), + renameStillActive: !!app._activeRename, + }; + }); + + expect(result.inputStillInDom).toBe(true); + expect(result.renameStillActive).toBe(true); + }); + + it('CJK guard: Enter with legacy keyCode 229 does not commit', async () => { + await resetState(); + expect(await startRename('cjk-229', 'OldName')).toBe(true); + + const renameStillActive = await page.evaluate(() => { + const app = (window as unknown as { app: { _activeRename: unknown } }).app; + const input = document.querySelector('input.tab-rename-input') as HTMLInputElement; + // Some Safari/Edge versions report keyCode 229 with isComposing=false on the + // Enter that triggers compositionend — the legacy guard catches that case. + input.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter', keyCode: 229, bubbles: true })); + return !!app._activeRename; + }); + + expect(renameStillActive).toBe(true); + }); + + it('CJK guard: regular Enter (no IME) DOES commit', async () => { + await resetState(); + expect(await startRename('regular-enter', 'OldName')).toBe(true); + + // Stub fetch so the commit doesn't hit the real API. + const result = await page.evaluate(async () => { + const app = (window as unknown as { app: { _activeRename: unknown } }).app; + let fetchUrl: string | null = null; + const origFetch = window.fetch; + window.fetch = (async (input: RequestInfo | URL) => { + fetchUrl = String(input); + return new Response('{"success":true}', { status: 200 }); + }) as typeof window.fetch; + + const inputEl = document.querySelector('input.tab-rename-input') as HTMLInputElement; + inputEl.value = 'NewName'; + inputEl.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter', bubbles: true })); + // Enter calls input.blur() which fires the async finishRename. Wait for it. + await new Promise((r) => setTimeout(r, 30)); + + window.fetch = origFetch; + return { fetchUrl, renameActive: !!app._activeRename }; + }); + + expect(result.fetchUrl).toContain('/api/sessions/regular-enter/name'); + expect(result.renameActive).toBe(false); + }); + + it('Ghost tab: _cleanupSessionData cancels rename for the deleted session', async () => { + await resetState(); + expect(await startRename('ghost-id', 'OldName')).toBe(true); + + const result = await page.evaluate(async () => { + const app = ( + window as unknown as { + app: { + _activeRename: { sessionId: string } | null; + sessions: Map; + _cleanupSessionData: (id: string) => void; + }; + } + ).app; + + let fetchFired = false; + const origFetch = window.fetch; + window.fetch = (async (input: RequestInfo | URL) => { + if (String(input).includes('/api/sessions/ghost-id/name')) fetchFired = true; + return new Response('{}', { status: 200 }); + }) as typeof window.fetch; + + const matchedBefore = app._activeRename?.sessionId === 'ghost-id'; + app._cleanupSessionData('ghost-id'); + // Cancel triggers async renderSessionTabs; allow it to settle. + await new Promise((r) => setTimeout(r, 50)); + + window.fetch = origFetch; + return { + matchedBefore, + renameActiveAfter: !!app._activeRename, + sessionGone: !app.sessions.has('ghost-id'), + fetchFired, + }; + }); + + expect(result.matchedBefore).toBe(true); + expect(result.renameActiveAfter).toBe(false); + expect(result.sessionGone).toBe(true); + // Cancel path skips the API call — deleting a session shouldn't trigger a stale rename PUT. + expect(result.fetchFired).toBe(false); + }); + + it('Ghost tab: _cleanupSessionData for a DIFFERENT session does NOT cancel rename', async () => { + await resetState(); + expect(await startRename('keep-rename', 'OldName')).toBe(true); + + const result = await page.evaluate(() => { + const app = ( + window as unknown as { + app: { + _activeRename: unknown; + sessions: Map; + _cleanupSessionData: (id: string) => void; + }; + } + ).app; + // Add an unrelated session and delete it — the rename for keep-rename must survive. + app.sessions.set('unrelated', { id: 'unrelated', name: 'X' }); + app._cleanupSessionData('unrelated'); + return { renameStillActive: !!app._activeRename }; + }); + + expect(result.renameStillActive).toBe(true); + }); + + it('Settle-once: cancel() is idempotent and clears _activeRename', async () => { + await resetState(); + expect(await startRename('idempotent-id', 'OldName')).toBe(true); + + const result = await page.evaluate(async () => { + const app = (window as unknown as { app: { _activeRename: { cancel: () => void } | null } }).app; + const cancelFn = app._activeRename!.cancel; + cancelFn(); + const afterFirst = app._activeRename; + let threw = false; + try { + cancelFn(); + } catch { + threw = true; + } + // Allow any async re-renders to settle. + await new Promise((r) => setTimeout(r, 30)); + const afterSecond = app._activeRename; + return { afterFirstNull: afterFirst === null, afterSecondNull: afterSecond === null, threw }; + }); + + expect(result.afterFirstNull).toBe(true); + expect(result.afterSecondNull).toBe(true); + expect(result.threw).toBe(false); + }); + + it('Re-entry: starting rename while one is active aborts the previous one', async () => { + await resetState(); + expect(await startRename('first-id', 'First')).toBe(true); + + const result = await page.evaluate(() => { + const app = ( + window as unknown as { + app: { + _activeRename: { sessionId: string } | null; + sessions: Map; + startInlineRename: (id: string) => void; + }; + } + ).app; + const firstActive = app._activeRename?.sessionId; + // Start a second rename without cancelling — startInlineRename should + // pre-emptively cancel the previous one so state never gets stuck on the dead session. + app.sessions.set('second-id', { id: 'second-id', name: 'Second' }); + const wrap = document.createElement('div'); + wrap.setAttribute('data-test-tab', '1'); + const tabName = document.createElement('span'); + tabName.className = 'tab-name'; + tabName.setAttribute('data-session-id', 'second-id'); + tabName.textContent = 'Second'; + wrap.appendChild(tabName); + document.body.appendChild(wrap); + app.startInlineRename('second-id'); + return { firstActive, secondActive: app._activeRename?.sessionId }; + }); + + expect(result.firstActive).toBe('first-id'); + expect(result.secondActive).toBe('second-id'); + }); +}); diff --git a/test/mobile/helpers/server.ts b/test/mobile/helpers/server.ts index 204aca3f..ffa4e58f 100644 --- a/test/mobile/helpers/server.ts +++ b/test/mobile/helpers/server.ts @@ -1,4 +1,4 @@ -import { WebServer } from '../../src/web/server.js'; +import { WebServer } from '../../../src/web/server.js'; let servers: Map = new Map();