From 92f51fa6194240802a1625367b75221925aba146 Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Sun, 4 Oct 2026 20:12:36 -0400 Subject: [PATCH] fix(tabs): inline rename review fixes Reopening the editor over a rename still in flight filled it from the name the server had not replaced yet, so dismissing it (blur commits) queued the old name behind the new one and undid the rename. The queue now records the newest queued name per session (_inlineRenamePending, cleared with the queue entry), and a reopened editor takes its prefix, input and "unchanged" comparison from it. An untouched confirm sends nothing more. A failed write only toasted while its editor was still current. The queue reports the failure itself now, and the editor only puts its label back. One rejected task blocked every later rename of that session until reload. Each task now chains from a settled predecessor, the local apply after a successful PUT is guarded, and the queue entry is cleaned up on either outcome. The rail and sidebar editor's 4rem floor moves from a stylesheet `!important` into the inline min-width startInlineRename already writes per layout (0 in the header strip, 4rem in the rail and sidebar). Tests: the reopened-editor case now expects only "First" to be sent; new cases cover a 500 answered after the editor is gone and a throw in updateSubagentParentNames; the long-prefix check runs in the sidebar and detailed sidebar too and asserts the inline floor; the header strip editor keeps min-width 0. --- src/web/public/session-ui.js | 62 +++++++++++----- src/web/public/styles.css | 5 +- test/inline-rename.test.ts | 135 +++++++++++++++++++++++++++++++++-- 3 files changed, 175 insertions(+), 27 deletions(-) diff --git a/src/web/public/session-ui.js b/src/web/public/session-ui.js index 4bce2273..a5036056 100644 --- a/src/web/public/session-ui.js +++ b/src/web/public/session-ui.js @@ -2596,12 +2596,22 @@ Object.assign(CodemanApp.prototype, { * in the editor: a confirmed name is applied locally even after its editor is * gone, and the "already that name" check runs only once the earlier writes * have landed, so confirming the name still on screen is a real write. - * Resolves { status: 'confirmed' | 'failed' | 'deleted' }; never rejects. + * Resolves { status: 'confirmed' | 'failed' | 'deleted' }; never rejects, + * and reports a failed write itself, since its editor may be gone by then. + * `_inlineRenamePending` holds the newest queued name per session, so an + * editor reopened over a write in flight starts from that name rather than + * the one the server has not replaced yet. */ _queueInlineSessionName(sessionId, desiredName) { this._inlineRenameWrites ??= new Map(); + this._inlineRenamePending ??= new Map(); const writes = this._inlineRenameWrites; - const task = (writes.get(sessionId) || Promise.resolve()).then(async () => { + const pending = this._inlineRenamePending; + pending.set(sessionId, desiredName); + // Chained from a settled promise, so one rejected write cannot stop the + // writes queued behind it. + const prev = (writes.get(sessionId) || Promise.resolve()).catch(() => {}); + const task = prev.then(async () => { const session = this.sessions.get(sessionId); if (!session) return { status: 'deleted' }; if (session.name === desiredName) return { status: 'confirmed' }; @@ -2612,15 +2622,26 @@ Object.assign(CodemanApp.prototype, { // A failure is a value, so a later write in the chain still runs. } if (!this.sessions.has(sessionId)) return { status: 'deleted' }; - if (confirmed === null) return { status: 'failed' }; - this._applyLocalSessionName(sessionId, confirmed); - this.renderSessionTabs(); + if (confirmed === null) { + this.showToast('Failed to rename', 'error'); + return { status: 'failed' }; + } + try { + this._applyLocalSessionName(sessionId, confirmed); + this.renderSessionTabs(); + } catch (err) { + // The server holds the name; a local repaint failing is not a failed write. + console.error('[rename] applying the confirmed name failed', err); + } return { status: 'confirmed' }; }); writes.set(sessionId, task); - task.then(() => { - if (writes.get(sessionId) === task) writes.delete(sessionId); - }); + const cleanup = () => { + if (writes.get(sessionId) !== task) return; + writes.delete(sessionId); + pending.delete(sessionId); + }; + task.then(cleanup, cleanup); return task; }, @@ -2912,7 +2933,10 @@ Object.assign(CodemanApp.prototype, { tabName.classList.add('tab-name-renaming'); const currentName = this.getSessionName(session); - const parsed = parseSessionPrefix(session.name); + // A rename still in flight is the user's last word, not the name the + // server has yet to replace: start from it, and compare against it below. + const shownName = this._inlineRenamePending?.get(sessionId) ?? session.name; + const parsed = parseSessionPrefix(shownName); const originalContent = tabName.textContent; const originalChildren = [...tabName.childNodes].map((node) => node.cloneNode(true)); const restoreOriginalChildren = () => { @@ -2933,13 +2957,17 @@ Object.assign(CodemanApp.prototype, { const input = document.createElement('input'); input.type = 'text'; - input.value = parsed ? parsed.suffix : (session.name || ''); + input.value = parsed ? parsed.suffix : (shownName || ''); input.placeholder = parsed ? 'Add description...' : currentName; input.className = 'tab-rename-input'; // 80px is tuned for the narrow header tab; a full-width sidebar row can and - // should give the whole line to the input. - const renameWidth = tabName.closest('.tab-rail') ? 'auto' : this.isSessionSidebarActive?.() ? '100%' : '80px'; - input.style.cssText = `width: ${renameWidth}; min-width: 0; font-size: 0.75rem; padding: 2px 4px; background: var(--bg-input); border: 1px solid var(--accent); border-radius: 3px; color: var(--text); outline: none;`; + // should give the whole line to the input. The header editor may shrink to + // nothing, while a rail or sidebar row always keeps room to type. + const inRail = !!tabName.closest('.tab-rail'); + const inSidebar = !inRail && !!this.isSessionSidebarActive?.(); + const renameWidth = inRail ? 'auto' : inSidebar ? '100%' : '80px'; + const renameMinWidth = inRail || inSidebar ? '4rem' : '0'; + input.style.cssText = `width: ${renameWidth}; min-width: ${renameMinWidth}; font-size: 0.75rem; padding: 2px 4px; background: var(--bg-input); border: 1px solid var(--accent); border-radius: 3px; color: var(--text); outline: none;`; tabName.appendChild(input); input.focus(); @@ -2989,7 +3017,7 @@ Object.assign(CodemanApp.prototype, { const suffix = input.value.trim(); const fullName = parsed ? parsed.prefix + (suffix ? ': ' + suffix : '') : suffix; - if (fullName === session.name) restoreOriginalChildren(); + if (fullName === shownName) restoreOriginalChildren(); else tabName.textContent = fullName || originalContent; // Skip the API call if the session vanished between focus and blur. The @@ -2998,10 +3026,8 @@ Object.assign(CodemanApp.prototype, { if (this.sessions.has(sessionId)) { const result = await this._queueInlineSessionName(sessionId, fullName); if (invalidated || this._activeRename !== renameHandle || !this.sessions.has(sessionId)) return; - if (result.status === 'failed') { - restoreOriginalChildren(); - this.showToast('Failed to rename', 'error'); - } + // The queue reports a failure itself; the editor only puts its label back. + if (result.status === 'failed') restoreOriginalChildren(); } // Re-render tabs to restore full tab structure completeCurrentRename(); diff --git a/src/web/public/styles.css b/src/web/public/styles.css index f3ef5797..8ba70b26 100644 --- a/src/web/public/styles.css +++ b/src/web/public/styles.css @@ -1867,15 +1867,14 @@ html[data-tab-orientation='vertical'] .tab-rail .session-tab .tab-name-prefix { text-overflow: ellipsis; } -/* The input always keeps room to type. `!important` beats the editor's inline - `min-width: 0`, which the header strip's fixed-width editor relies on. */ +/* The input takes the rest of the row. Its 4rem floor is the editor's own + inline min-width (startInlineRename picks it per layout). */ :is( html[data-tab-orientation='vertical'] .tab-rail, html[data-session-list='sidebar'] .session-sidebar ) .tab-name-renaming .tab-rename-input { flex: 1 1 0; width: auto; - min-width: 4rem !important; } /* A compact rail row has no room for both the editor and its adornments, so diff --git a/test/inline-rename.test.ts b/test/inline-rename.test.ts index 82e7db40..e851f310 100644 --- a/test/inline-rename.test.ts +++ b/test/inline-rename.test.ts @@ -802,16 +802,133 @@ describe('Inline rename write ordering', () => { expect(await state('reopen')).toEqual({ bodies: ['First'], mapName: 'First', renameActive: false }); }); - it('re-sends the shown name when it is confirmed unchanged over a rename still in flight', async () => { + it('reopens the editor on the name still in flight, so confirming it unchanged keeps the rename', async () => { await mount('stale', 'Old'); await commit('stale', 'First'); - // The reopened editor still shows "Old" (the PUT has not answered), and - // Enter confirms that: the user's last word is "Old", not "First". + // The PUT for "First" has not answered, so app.sessions still says "Old". + // The reopened editor must show "First", the user's last word, and an + // untouched confirm must not queue "Old" behind it. + const reopenedValue = await page.evaluate(() => { + (window as unknown as { app: { startInlineRename: (id: string) => void } }).app.startInlineRename('stale'); + return (document.querySelector('.tab-name[data-session-id="stale"] input.tab-rename-input') as HTMLInputElement) + .value; + }); + expect(reopenedValue).toBe('First'); await commit('stale', null); await answer(0, 'First'); - await answer(1, 'Old'); await restoreFetch(); - expect(await state('stale')).toEqual({ bodies: ['First', 'Old'], mapName: 'Old', renameActive: false }); + expect(await state('stale')).toEqual({ bodies: ['First'], mapName: 'First', renameActive: false }); + expect( + await page.evaluate( + () => + (window as unknown as { app: { _inlineRenamePending?: Map } }).app._inlineRenamePending?.has( + 'stale' + ) ?? false + ) + ).toBe(false); + }); + + it('reports a failed write even after its editor is gone', async () => { + await mount('fail-late', 'Old'); + await page.evaluate(() => { + const w = window as unknown as { + app: { showToast: (message: string, type?: string) => void }; + __toasts: string[]; + __origToast?: (message: string, type?: string) => void; + }; + w.__toasts = []; + w.__origToast = w.app.showToast; + w.app.showToast = (message: string) => { + w.__toasts.push(message); + }; + }); + await commit('fail-late', 'First'); + // Reopen and dismiss: the editor that made the write is gone. + await page.evaluate(() => { + const app = ( + window as unknown as { app: { startInlineRename: (id: string) => void; _activeRename: { cancel: () => void } } } + ).app; + app.startInlineRename('fail-late'); + app._activeRename.cancel(); + }); + await page.evaluate(async () => { + const w = window as unknown as { __pending: Pending[] }; + w.__pending[0]?.resolve( + new Response(JSON.stringify({ success: false, error: 'boom' }), { + status: 500, + headers: { 'Content-Type': 'application/json' }, + }) + ); + await new Promise((resolve) => setTimeout(resolve, 30)); + }); + await restoreFetch(); + const toasts = await page.evaluate(() => { + const w = window as unknown as { + app: { showToast: unknown }; + __toasts: string[]; + __origToast?: unknown; + }; + w.app.showToast = w.__origToast; + return w.__toasts; + }); + expect(toasts).toEqual(['Failed to rename']); + expect(await state('fail-late')).toEqual({ bodies: ['First'], mapName: 'Old', renameActive: false }); + }); + + it('keeps sending a session renames after the work following a PUT throws', async () => { + await mount('throws', 'Old'); + await page.evaluate(() => { + const w = window as unknown as { + app: { updateSubagentParentNames?: (id: string) => void }; + __origParentNames?: (id: string) => void; + __throwOnce: boolean; + }; + w.__origParentNames = w.app.updateSubagentParentNames; + w.__throwOnce = true; + w.app.updateSubagentParentNames = (id: string) => { + if (w.__throwOnce) { + w.__throwOnce = false; + throw new Error('forced'); + } + w.__origParentNames?.call(w.app, id); + }; + window.addEventListener('unhandledrejection', (event) => event.preventDefault(), { once: true }); + }); + await commit('throws', 'First'); + await answer(0, 'First'); + await commit('throws', 'Second'); + await answer(1, 'Second'); + await restoreFetch(); + const leftover = await page.evaluate(() => { + const w = window as unknown as { + app: { updateSubagentParentNames?: unknown; _inlineRenameWrites?: Map }; + __origParentNames?: unknown; + }; + w.app.updateSubagentParentNames = w.__origParentNames; + return w.app._inlineRenameWrites?.has('throws') ?? false; + }); + expect(await state('throws')).toEqual({ bodies: ['First', 'Second'], mapName: 'Second', renameActive: false }); + expect(leftover).toBe(false); + }); + + it('lets the header strip editor shrink (inline min-width 0)', async () => { + await mount('header-width', 'Old'); + const minWidth = await page.evaluate(() => { + const app = ( + window as unknown as { + app: { startInlineRename: (id: string) => void; _activeRename: { cancel: () => void } | null }; + } + ).app; + app.startInlineRename('header-width'); + const input = document.querySelector( + '.tab-name[data-session-id="header-width"] input.tab-rename-input' + ) as HTMLInputElement; + const value = input.style.minWidth; + app._activeRename?.cancel(); + return value; + }); + await restoreFetch(); + expect(minWidth).toBe('0px'); }); it('keeps a confirmed session rename when a group rename takes over the editor', async () => { @@ -901,6 +1018,8 @@ describe('Vertical rail rename editor with a long prefix', () => { { variant: 'simple rows', settings: { tabOrientation: 'vertical', tabRailDetail: 'simple' }, compact: false }, { variant: 'detailed rows', settings: { tabOrientation: 'vertical', tabRailDetail: 'rich' }, compact: false }, { variant: 'compact rail', settings: { tabOrientation: 'vertical', tabRailWidth: 208 }, compact: true }, + { variant: 'sidebar', settings: { sessionListLayout: 'sidebar' }, compact: false }, + { variant: 'detailed sidebar', settings: { sessionListLayout: 'sidebar-rich' }, compact: false }, ])('keeps the prefix and a usable input inside the row ($variant)', async ({ settings, compact }) => { const context = await browser.newContext({ viewport: { width: 1280, height: 720 }, deviceScaleFactor: 1 }); try { @@ -910,7 +1029,8 @@ describe('Vertical rail rename editor with a long prefix', () => { ); const page = await context.newPage(); await page.goto(`http://localhost:${port}`, { waitUntil: 'domcontentloaded' }); - const row = page.locator(`#tabRail .session-tab[data-id="${sessionId}"]`); + // One #sessionTabs list, moved into the rail or the sidebar by the layout. + const row = page.locator(`#sessionTabs .session-tab[data-id="${sessionId}"]`); await row.waitFor({ state: 'visible', timeout: 15000 }); expect(await page.evaluate(() => document.documentElement.classList.contains('tab-rail-compact'))).toBe(compact); @@ -937,6 +1057,7 @@ describe('Vertical rail rename editor with a long prefix', () => { inputInsideRow: within(box(input), box(info)), prefixInsideRow: within(box(prefix), box(info)), prefixEllipsis: getComputedStyle(prefix).textOverflow, + inlineMinWidth: input.style.minWidth, }; }); expect(geometry.focused).toBe(true); @@ -945,6 +1066,8 @@ describe('Vertical rail rename editor with a long prefix', () => { expect(geometry.inputInsideRow).toBe(true); expect(geometry.prefixInsideRow).toBe(true); expect(geometry.prefixEllipsis).toBe('ellipsis'); + // The floor is the editor's own inline style, not a stylesheet override. + expect(geometry.inlineMinWidth).toBe('4rem'); await input.press('Escape'); expect(await row.locator('input.tab-rename-input').count()).toBe(0);