From 33f77c4680eebae3b05015463b4b342defa78542 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Tue, 25 Aug 2026 18:26:07 +0200 Subject: [PATCH] =?UTF-8?q?fix(tabs):=20review=20fixes=20for=20the=20detai?= =?UTF-8?q?led=20rail=20=E2=80=94=20width-dialog=20default,=20compact=20wr?= =?UTF-8?q?ap=20pass,=20rich-aware=20resets?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings on the detailed-rows feature, all in its edge cases: - The App Settings width select consulted the handheld defaults blob (tabRailWidth: 256) BEFORE the rich-aware default, which the renderer never reads — so a tablet's unsized rich rail rendered 320 while the dialog said 256, and a routine Save persisted the 256 (below the 288px tight threshold, permanently). The chain now mirrors applyTabRailWidth()'s actual resolution. - _setTabRailWidth() re-rendered on a compact flip but never re-ran applyTabWrapSettings(), the one owner of the folder line, whose railRich input reads the compact class this function just toggled. A rich rail dragged below 240px kept emitting folder rows — persistently, for a stored width < 240, since the boot wrap pass runs before the class is first applied. The wrap pass now re-runs on the flip, with exactly one render either way. - Both reset affordances (handle dblclick, Enter on the handle) reset to the hardcoded 256 even on a rich rail, landing it below the tight threshold; both now resolve the rich-aware default (320), via a new optional defaultWidth input on resolveTabRailKeyboardWidth(). Co-Authored-By: Claude Fable 5 --- src/web/public/constants.js | 4 ++- src/web/public/settings-ui.js | 8 +++-- src/web/public/tab-rail-resize.js | 21 +++++++++-- test/tab-rail-resize.test.ts | 59 +++++++++++++++++++++++++++++++ 4 files changed, 87 insertions(+), 5 deletions(-) diff --git a/src/web/public/constants.js b/src/web/public/constants.js index 01d679bf..45579ca1 100644 --- a/src/web/public/constants.js +++ b/src/web/public/constants.js @@ -188,7 +188,9 @@ function resolveTabRailKeyboardWidth(input = {}) { let width; if (input.key === 'Home') width = TAB_RAIL_MIN_WIDTH; else if (input.key === 'End') width = TAB_RAIL_MAX_WIDTH; - else if (input.key === 'Enter') width = TAB_RAIL_DEFAULT_WIDTH; + // Enter resets to the caller's effective default (the rich rail's is the + // Wide preset, not 256 — see _defaultTabRailWidth); absent, the base default. + else if (input.key === 'Enter') width = Number(input.defaultWidth) || TAB_RAIL_DEFAULT_WIDTH; else if (input.key === 'ArrowLeft' || input.key === 'ArrowRight') { const direction = input.key === 'ArrowLeft' ? -1 : 1; width = (Number(input.currentWidth) || TAB_RAIL_DEFAULT_WIDTH) + direction * (input.shiftKey ? 32 : 8); diff --git a/src/web/public/settings-ui.js b/src/web/public/settings-ui.js index 5f07f3a8..005c94e6 100644 --- a/src/web/public/settings-ui.js +++ b/src/web/public/settings-ui.js @@ -405,8 +405,12 @@ Object.assign(CodemanApp.prototype, { const tabRailWidth = window.CodemanTabRail?.resolveWidth({ // Same default resolution as applyTabRailWidth(): a rail that has never // been sized shows the width it is actually rendering at, which for - // detailed rows is the Wide preset rather than 256. - width: settings.tabRailWidth ?? defaults.tabRailWidth ?? this._defaultTabRailWidth?.() ?? 256, + // detailed rows is the Wide preset rather than 256. The rich-aware + // default must come BEFORE the per-device defaults blob: the handheld + // blob carries tabRailWidth: 256, which applyTabRailWidth() never reads, + // so consulting it first showed a tablet's unsized rich rail as 256 while + // it rendered at 320 — and a routine Save then PERSISTED the 256. + width: settings.tabRailWidth ?? this._defaultTabRailWidth?.() ?? defaults.tabRailWidth ?? 256, }) ?? 256; this.syncTabRailWidthSetting?.(tabRailWidth); document.getElementById('appSettingsTabRailDetail').value = diff --git a/src/web/public/tab-rail-resize.js b/src/web/public/tab-rail-resize.js index 38947ea9..ab2e54ab 100644 --- a/src/web/public/tab-rail-resize.js +++ b/src/web/public/tab-rail-resize.js @@ -76,7 +76,21 @@ Object.assign(CodemanApp.prototype, { // list is ordered by) and its pill intact. No re-render — unlike the rows // themselves, this is a display toggle on markup that is already there. root.classList.toggle('tab-rail-tight', resolved < 288); - if (wasCompact !== compact) this._fullRenderSessionTabs?.(); + if (wasCompact !== compact) { + // The folder line is owned by applyTabWrapSettings(), whose railRich + // input reads the compact class this function just toggled — without + // re-running it, a rich rail dragged below 240px kept emitting folder + // rows (and, for a stored width < 240, kept them across reloads: the + // boot-time wrap pass runs before this function first applies the + // class). It re-renders only when the folder flag actually flipped, so + // cover the flip-without-folder-change case (a simple-detail rail + // crossing 240px still changes the row-action affordance) without + // rendering twice. + const prevTall = this._tallTabsEnabled; + this.applyTabWrapSettings?.(); + const wrapRendered = prevTall !== undefined && this._tallTabsEnabled !== prevTall; + if (!wrapRendered) this._fullRenderSessionTabs?.(); + } const handle = document.getElementById('tabRailResizeHandle'); if (handle) { handle.setAttribute('aria-valuemax', String(effectiveMax)); @@ -223,6 +237,7 @@ Object.assign(CodemanApp.prototype, { key: event.key, shiftKey: event.shiftKey, currentWidth: this._getCurrentTabRailWidth(), + defaultWidth: this._defaultTabRailWidth?.(), ...this._getTabRailBounds(), }); if (width === null || width === undefined) return; @@ -265,7 +280,9 @@ Object.assign(CodemanApp.prototype, { handle.addEventListener('dblclick', (event) => { event.preventDefault(); this._claimTabRailResize(); - const preferred = window.CodemanTabRail?.DEFAULT_WIDTH || 256; + // Rich-aware: resetting a detailed rail to 256 would land it below the + // 288px tight threshold and silently drop the created stamp. + const preferred = this._defaultTabRailWidth?.() ?? (window.CodemanTabRail?.DEFAULT_WIDTH || 256); const effective = this._setTabRailWidth(preferred); this._scheduleTabRailSettle(effective, preferred); }); diff --git a/test/tab-rail-resize.test.ts b/test/tab-rail-resize.test.ts index 365124d7..8d5a3c36 100644 --- a/test/tab-rail-resize.test.ts +++ b/test/tab-rail-resize.test.ts @@ -53,6 +53,10 @@ describe('tab rail width policy', () => { expect(policy.resolveKeyboardWidth({ ...base, key: 'Home' })).toBe(208); expect(policy.resolveKeyboardWidth({ ...base, key: 'End' })).toBe(360); expect(policy.resolveKeyboardWidth({ ...base, key: 'Enter' })).toBe(256); + // Enter resets to the CALLER's effective default: a rich rail passes 320 + // (its unsized rendering width), so the reset cannot land it below the + // 288px tight threshold the way a hardcoded 256 did. + expect(policy.resolveKeyboardWidth({ ...base, key: 'Enter', defaultWidth: 320 })).toBe(320); expect(policy.resolveKeyboardWidth({ ...base, key: 'Escape' })).toBeNull(); }); }); @@ -123,6 +127,61 @@ describe('tab rail resize wiring', () => { expect(app._tabRailResizeOwnsObserver).toBe(false); }); + it('re-runs the wrap pass when the compact threshold flips, without double-rendering', () => { + const controller = readPublic('tab-rail-resize.js'); + class FakeCodemanApp {} + const classes = new Set(); + const context = vm.createContext({ + CodemanApp: FakeCodemanApp, + window: { CodemanTabRail: loadRailPolicy() }, + document: { + documentElement: { + style: { setProperty: () => {} }, + classList: { + contains: (c: string) => classes.has(c), + toggle: (c: string, force: boolean) => { + if (force) classes.add(c); + else classes.delete(c); + return force; + }, + }, + }, + getElementById: () => null, + querySelector: () => null, + }, + console, + clearTimeout, + setTimeout, + }); + vm.runInContext(controller, context, { filename: 'tab-rail-resize.js' }); + const app = new FakeCodemanApp() as FakeCodemanApp & Record; + app._getTabRailBounds = () => ({}); + app.syncTabRailWidthSetting = vi.fn(); + app._fullRenderSessionTabs = vi.fn(); + + // Rich rail dragged below 240px: applyTabWrapSettings() owns the folder + // line and reads the compact class this call just toggled, so it must be + // re-consulted on the flip — and when its own conditional render fires + // (the folder flag changed), the explicit render must not double it. + app._tallTabsEnabled = true; + app.applyTabWrapSettings = vi.fn(() => { + app._tallTabsEnabled = false; + app._fullRenderSessionTabs(); + }); + app._setTabRailWidth(210); + expect(app.applyTabWrapSettings).toHaveBeenCalledOnce(); + expect(app._fullRenderSessionTabs).toHaveBeenCalledOnce(); + + // Flip back up with an unchanged folder flag (simple-detail rail): the + // explicit render must still fire — the compact row-action affordance + // changed even though the wrap pass rendered nothing. + app.applyTabWrapSettings = vi.fn(); + app._fullRenderSessionTabs = vi.fn(); + app._setTabRailWidth(300); + expect(app.applyTabWrapSettings).toHaveBeenCalledOnce(); + expect(app._fullRenderSessionTabs).toHaveBeenCalledOnce(); + }); + it('keeps resize-observer ownership for pointer drags longer than the watchdog', async () => { vi.useFakeTimers(); const controller = readPublic('tab-rail-resize.js');