mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-09 00:49:41 +02:00
fix(tabs): review fixes for the detailed rail — width-dialog default, compact wrap pass, rich-aware resets
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 <noreply@anthropic.com>
This commit is contained in:
@@ -188,7 +188,9 @@ function resolveTabRailKeyboardWidth(input = {}) {
|
|||||||
let width;
|
let width;
|
||||||
if (input.key === 'Home') width = TAB_RAIL_MIN_WIDTH;
|
if (input.key === 'Home') width = TAB_RAIL_MIN_WIDTH;
|
||||||
else if (input.key === 'End') width = TAB_RAIL_MAX_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') {
|
else if (input.key === 'ArrowLeft' || input.key === 'ArrowRight') {
|
||||||
const direction = input.key === 'ArrowLeft' ? -1 : 1;
|
const direction = input.key === 'ArrowLeft' ? -1 : 1;
|
||||||
width = (Number(input.currentWidth) || TAB_RAIL_DEFAULT_WIDTH) + direction * (input.shiftKey ? 32 : 8);
|
width = (Number(input.currentWidth) || TAB_RAIL_DEFAULT_WIDTH) + direction * (input.shiftKey ? 32 : 8);
|
||||||
|
|||||||
@@ -405,8 +405,12 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
const tabRailWidth = window.CodemanTabRail?.resolveWidth({
|
const tabRailWidth = window.CodemanTabRail?.resolveWidth({
|
||||||
// Same default resolution as applyTabRailWidth(): a rail that has never
|
// Same default resolution as applyTabRailWidth(): a rail that has never
|
||||||
// been sized shows the width it is actually rendering at, which for
|
// been sized shows the width it is actually rendering at, which for
|
||||||
// detailed rows is the Wide preset rather than 256.
|
// detailed rows is the Wide preset rather than 256. The rich-aware
|
||||||
width: settings.tabRailWidth ?? defaults.tabRailWidth ?? this._defaultTabRailWidth?.() ?? 256,
|
// 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;
|
}) ?? 256;
|
||||||
this.syncTabRailWidthSetting?.(tabRailWidth);
|
this.syncTabRailWidthSetting?.(tabRailWidth);
|
||||||
document.getElementById('appSettingsTabRailDetail').value =
|
document.getElementById('appSettingsTabRailDetail').value =
|
||||||
|
|||||||
@@ -76,7 +76,21 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
// list is ordered by) and its pill intact. No re-render — unlike the rows
|
// 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.
|
// themselves, this is a display toggle on markup that is already there.
|
||||||
root.classList.toggle('tab-rail-tight', resolved < 288);
|
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');
|
const handle = document.getElementById('tabRailResizeHandle');
|
||||||
if (handle) {
|
if (handle) {
|
||||||
handle.setAttribute('aria-valuemax', String(effectiveMax));
|
handle.setAttribute('aria-valuemax', String(effectiveMax));
|
||||||
@@ -223,6 +237,7 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
key: event.key,
|
key: event.key,
|
||||||
shiftKey: event.shiftKey,
|
shiftKey: event.shiftKey,
|
||||||
currentWidth: this._getCurrentTabRailWidth(),
|
currentWidth: this._getCurrentTabRailWidth(),
|
||||||
|
defaultWidth: this._defaultTabRailWidth?.(),
|
||||||
...this._getTabRailBounds(),
|
...this._getTabRailBounds(),
|
||||||
});
|
});
|
||||||
if (width === null || width === undefined) return;
|
if (width === null || width === undefined) return;
|
||||||
@@ -265,7 +280,9 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
handle.addEventListener('dblclick', (event) => {
|
handle.addEventListener('dblclick', (event) => {
|
||||||
event.preventDefault();
|
event.preventDefault();
|
||||||
this._claimTabRailResize();
|
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);
|
const effective = this._setTabRailWidth(preferred);
|
||||||
this._scheduleTabRailSettle(effective, preferred);
|
this._scheduleTabRailSettle(effective, preferred);
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -53,6 +53,10 @@ describe('tab rail width policy', () => {
|
|||||||
expect(policy.resolveKeyboardWidth({ ...base, key: 'Home' })).toBe(208);
|
expect(policy.resolveKeyboardWidth({ ...base, key: 'Home' })).toBe(208);
|
||||||
expect(policy.resolveKeyboardWidth({ ...base, key: 'End' })).toBe(360);
|
expect(policy.resolveKeyboardWidth({ ...base, key: 'End' })).toBe(360);
|
||||||
expect(policy.resolveKeyboardWidth({ ...base, key: 'Enter' })).toBe(256);
|
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();
|
expect(policy.resolveKeyboardWidth({ ...base, key: 'Escape' })).toBeNull();
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
@@ -123,6 +127,61 @@ describe('tab rail resize wiring', () => {
|
|||||||
expect(app._tabRailResizeOwnsObserver).toBe(false);
|
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<string>();
|
||||||
|
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<string, any>;
|
||||||
|
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 () => {
|
it('keeps resize-observer ownership for pointer drags longer than the watchdog', async () => {
|
||||||
vi.useFakeTimers();
|
vi.useFakeTimers();
|
||||||
const controller = readPublic('tab-rail-resize.js');
|
const controller = readPublic('tab-rail-resize.js');
|
||||||
|
|||||||
Reference in New Issue
Block a user