mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-02 13:39:41 +02:00
fix(mobile): make every session tab reachable in the tab strip (#257)
With five tabs open on a phone, the right-hand tabs were effectively unreachable. Selecting a tab only toggled the .active class, so the strip never moved, and every full rebuild (a task badge appearing, a session created elsewhere) replaced the strip's innerHTML, which resets scrollLeft to 0 and yanked a mid-swipe strip back to the first tab. Three changes, which only work together: * computeTabScrollLeft() (pure, constants.js) decides the scroll target from measured rects, and _scrollActiveTabIntoView() applies it on selection. Rect math on the strip's own scrollLeft rather than scrollIntoView(), which also scrolls ancestors: on a phone that is the document, under a fixed header and possibly an open keyboard. * _fullRenderSessionTabs() saves and restores scrollLeft across the rebuild, and re-reveals the active tab only when it actually changed (_lastRenderedActiveTabId), so a background render never undoes a manual swipe. * Mobile no longer hoists the active session to the front of the strip. That reordering ran on full renders only, so tab order flipped depending on which render path fired, and it renumbered the Alt+N badges. Scrolling the active tab into view replaces it. Also sets overscroll-behavior-x: contain on the strip so a swipe that runs past the last tab stays in the strip instead of becoming the browser's back gesture. Tests: scroll-target math in test/tab-overflow.test.ts (runs in CI), plus five browser regressions in test/mobile/tabs.test.ts covering reveal-on- select in both directions, scroll preservation across an ambient rebuild, sessionOrder rendering on phones, and a real touch drag reaching the last tab. Closes #257 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Codeman maintainer
@@ -183,6 +183,171 @@ describe('Tab Navigation', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ─── Tab Strip Scrolling (issue #257) ────────────────────────────────────
|
||||
|
||||
describe('Tab Strip Scrolling', () => {
|
||||
/**
|
||||
* Seed `count` real sessions and render the strip through the production
|
||||
* code path (_fullRenderSessionTabs), so the tabs carry the real markup,
|
||||
* widths and CSS rather than hand-built stand-ins.
|
||||
*/
|
||||
async function seedTabs(page: Page, count: number, activeIndex = 0): Promise<void> {
|
||||
await page.evaluate(`(function (n, activeIndex) {
|
||||
app.sessions.clear();
|
||||
app.sessionOrder = [];
|
||||
for (let i = 1; i <= n; i++) {
|
||||
const id = 'scroll-sess-' + i;
|
||||
app.sessions.set(id, { id, name: 'w' + i + '-project', status: 'idle', mode: 'claude', workingDir: '/tmp/p' + i });
|
||||
app.sessionOrder.push(id);
|
||||
}
|
||||
app.activeSessionId = app.sessionOrder[activeIndex];
|
||||
app._lastRenderedActiveTabId = null;
|
||||
app._fullRenderSessionTabs();
|
||||
})(${count}, ${activeIndex})`);
|
||||
await page.waitForTimeout(200);
|
||||
}
|
||||
|
||||
async function stripState(page: Page, sessionId: string) {
|
||||
return page.evaluate(`(function (id) {
|
||||
const c = document.getElementById('sessionTabs');
|
||||
const tab = c.querySelector('.session-tab[data-id="' + id + '"]');
|
||||
const cRect = c.getBoundingClientRect();
|
||||
const tRect = tab ? tab.getBoundingClientRect() : null;
|
||||
return {
|
||||
scrollLeft: Math.round(c.scrollLeft),
|
||||
maxScroll: Math.round(c.scrollWidth - c.clientWidth),
|
||||
order: [...c.querySelectorAll('.session-tab[data-id]')].map((t) => t.dataset.id),
|
||||
visible: tRect ? tRect.left >= cRect.left - 1 && tRect.right <= cRect.right + 1 : false,
|
||||
};
|
||||
})('${sessionId}')`) as Promise<{ scrollLeft: number; maxScroll: number; order: string[]; visible: boolean }>;
|
||||
}
|
||||
|
||||
it('reveals a rightmost tab that selection would otherwise leave off-screen', async () => {
|
||||
const { context, page } = await createDevicePage(standardPhone, BASE_URL, 'chromium');
|
||||
try {
|
||||
await page.waitForTimeout(WAIT.PAGE_SETTLE);
|
||||
await seedTabs(page, 5);
|
||||
|
||||
const before = await stripState(page, 'scroll-sess-5');
|
||||
// Precondition: the strip really does overflow and the last tab is hidden.
|
||||
expect(before.maxScroll).toBeGreaterThan(0);
|
||||
expect(before.visible).toBe(false);
|
||||
|
||||
// The selection path selectSession() uses (class toggle, no rebuild).
|
||||
await page.evaluate(`(function () {
|
||||
app.activeSessionId = 'scroll-sess-5';
|
||||
app._updateActiveTabImmediate('scroll-sess-5');
|
||||
})()`);
|
||||
await page.waitForTimeout(600); // smooth scroll
|
||||
|
||||
const after = await stripState(page, 'scroll-sess-5');
|
||||
expect(after.visible).toBe(true);
|
||||
expect(after.scrollLeft).toBeGreaterThan(before.scrollLeft);
|
||||
} finally {
|
||||
await context.close();
|
||||
}
|
||||
});
|
||||
|
||||
it('scrolls back to reveal a leftmost tab', async () => {
|
||||
const { context, page } = await createDevicePage(standardPhone, BASE_URL, 'chromium');
|
||||
try {
|
||||
await page.waitForTimeout(WAIT.PAGE_SETTLE);
|
||||
await seedTabs(page, 5);
|
||||
await page.evaluate(`document.getElementById('sessionTabs').scrollLeft = 9999`);
|
||||
|
||||
await page.evaluate(`(function () {
|
||||
app.activeSessionId = 'scroll-sess-1';
|
||||
app._updateActiveTabImmediate('scroll-sess-1');
|
||||
})()`);
|
||||
await page.waitForTimeout(600);
|
||||
|
||||
const after = await stripState(page, 'scroll-sess-1');
|
||||
expect(after.visible).toBe(true);
|
||||
expect(after.scrollLeft).toBe(0);
|
||||
} finally {
|
||||
await context.close();
|
||||
}
|
||||
});
|
||||
|
||||
it('keeps the scroll position across an ambient full re-render', async () => {
|
||||
const { context, page } = await createDevicePage(standardPhone, BASE_URL, 'chromium');
|
||||
try {
|
||||
await page.waitForTimeout(WAIT.PAGE_SETTLE);
|
||||
await seedTabs(page, 5);
|
||||
|
||||
// User swipes to the end of the strip, then a background rebuild fires
|
||||
// (a task badge appearing forces the full-render path).
|
||||
await page.evaluate(`document.getElementById('sessionTabs').scrollLeft = 9999`);
|
||||
const scrolled = await stripState(page, 'scroll-sess-5');
|
||||
expect(scrolled.scrollLeft).toBeGreaterThan(0);
|
||||
|
||||
await page.evaluate(`(function () {
|
||||
app.sessions.get('scroll-sess-2').taskStats = { running: 2, total: 3 };
|
||||
app._fullRenderSessionTabs();
|
||||
})()`);
|
||||
await page.waitForTimeout(200);
|
||||
|
||||
const after = await stripState(page, 'scroll-sess-5');
|
||||
expect(after.scrollLeft).toBe(scrolled.scrollLeft);
|
||||
} finally {
|
||||
await context.close();
|
||||
}
|
||||
});
|
||||
|
||||
it('renders tabs in sessionOrder on phones instead of hoisting the active one', async () => {
|
||||
const { context, page } = await createDevicePage(standardPhone, BASE_URL, 'chromium');
|
||||
try {
|
||||
await page.waitForTimeout(WAIT.PAGE_SETTLE);
|
||||
await seedTabs(page, 5, 3); // 4th tab active
|
||||
|
||||
const state = await stripState(page, 'scroll-sess-4');
|
||||
expect(state.order).toEqual([
|
||||
'scroll-sess-1',
|
||||
'scroll-sess-2',
|
||||
'scroll-sess-3',
|
||||
'scroll-sess-4',
|
||||
'scroll-sess-5',
|
||||
]);
|
||||
// ...and the active tab is still brought into view by the render.
|
||||
expect(state.visible).toBe(true);
|
||||
} finally {
|
||||
await context.close();
|
||||
}
|
||||
});
|
||||
|
||||
it('reaches the last tab with a horizontal touch drag', async () => {
|
||||
const { context, page } = await createDevicePage(standardPhone, BASE_URL, 'chromium');
|
||||
try {
|
||||
await page.waitForTimeout(WAIT.PAGE_SETTLE);
|
||||
await seedTabs(page, 5);
|
||||
|
||||
const cdp = await context.newCDPSession(page);
|
||||
const box = await page.locator(SELECTORS.TABS_CONTAINER).boundingBox();
|
||||
if (!box) throw new Error('tab strip not found');
|
||||
const y = box.y + box.height / 2;
|
||||
const startX = box.x + box.width * 0.85;
|
||||
const endX = box.x + box.width * 0.1;
|
||||
|
||||
await cdp.send('Input.dispatchTouchEvent', { type: 'touchStart', touchPoints: [{ x: startX, y }] });
|
||||
for (let i = 1; i <= 10; i++) {
|
||||
await cdp.send('Input.dispatchTouchEvent', {
|
||||
type: 'touchMove',
|
||||
touchPoints: [{ x: startX + ((endX - startX) * i) / 10, y }],
|
||||
});
|
||||
await page.waitForTimeout(16);
|
||||
}
|
||||
await cdp.send('Input.dispatchTouchEvent', { type: 'touchEnd', touchPoints: [] });
|
||||
await page.waitForTimeout(400);
|
||||
|
||||
const after = await stripState(page, 'scroll-sess-5');
|
||||
expect(after.scrollLeft).toBeGreaterThan(0);
|
||||
expect(after.visible).toBe(true);
|
||||
} finally {
|
||||
await context.close();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
// ─── Swipe Navigation (CDP - Chromium) ───────────────────────────────────
|
||||
|
||||
describe('Swipe Navigation (CDP - Chromium)', () => {
|
||||
|
||||
@@ -3,12 +3,28 @@ import { resolve } from 'node:path';
|
||||
import vm from 'node:vm';
|
||||
import { describe, expect, it } from 'vitest';
|
||||
|
||||
type ScrollInput = {
|
||||
scrollLeft?: number;
|
||||
clientWidth?: number;
|
||||
scrollWidth?: number;
|
||||
tabLeft?: number;
|
||||
tabWidth?: number;
|
||||
padding?: number;
|
||||
};
|
||||
|
||||
function loadTabOverflowHelper() {
|
||||
const context = vm.createContext({ window: {}, globalThis: {} });
|
||||
const source = readFileSync(resolve(import.meta.dirname, '../src/web/public/constants.js'), 'utf8');
|
||||
vm.runInContext(source, context, { filename: 'constants.js' });
|
||||
return (context.window as { CodemanTabOverflow: { shouldAutoWrapTabs: (input: unknown) => boolean } })
|
||||
.CodemanTabOverflow;
|
||||
return (
|
||||
context.window as {
|
||||
CodemanTabOverflow: {
|
||||
shouldAutoWrapTabs: (input: unknown) => boolean;
|
||||
computeTabScrollLeft: (input: ScrollInput) => number;
|
||||
TAB_SCROLL_REVEAL_PX: number;
|
||||
};
|
||||
}
|
||||
).CodemanTabOverflow;
|
||||
}
|
||||
|
||||
describe('tab overflow layout policy', () => {
|
||||
@@ -63,3 +79,64 @@ describe('tab overflow layout policy', () => {
|
||||
expect(helper.shouldAutoWrapTabs({ ...base, tabCount: 1, scrollWidth: 1400, clientWidth: 760 })).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
// Issue #257: the phone tab strip scrolls horizontally, so the active tab can
|
||||
// sit entirely outside the visible slice. These pin the scroll target math that
|
||||
// _scrollActiveTabIntoView() feeds with measured rects.
|
||||
describe('mobile tab strip scroll-into-view policy', () => {
|
||||
// A 5-tab phone strip: 335px visible of 558px of tabs.
|
||||
const strip = { clientWidth: 335, scrollWidth: 558 };
|
||||
const pad = 16;
|
||||
|
||||
it('scrolls right to reveal a tab past the right edge, leaving the reveal sliver', () => {
|
||||
const helper = loadTabOverflowHelper();
|
||||
// Last tab: 458..558, strip parked at 0.
|
||||
const target = helper.computeTabScrollLeft({ ...strip, scrollLeft: 0, tabLeft: 458, tabWidth: 100 });
|
||||
// 558 + 16 - 335 = 239, clamped to the 223px maximum.
|
||||
expect(target).toBe(223);
|
||||
// The revealed tab is now inside the window.
|
||||
expect(458).toBeGreaterThanOrEqual(target);
|
||||
expect(558).toBeLessThanOrEqual(target + strip.clientWidth);
|
||||
});
|
||||
|
||||
it('scrolls left to reveal a tab before the left edge', () => {
|
||||
const helper = loadTabOverflowHelper();
|
||||
// First tab: 0..150, strip scrolled to the end.
|
||||
expect(helper.computeTabScrollLeft({ ...strip, scrollLeft: 223, tabLeft: 0, tabWidth: 150 })).toBe(0);
|
||||
// A middle tab partially cut off on the left: reveal it with the sliver.
|
||||
expect(helper.computeTabScrollLeft({ ...strip, scrollLeft: 223, tabLeft: 200, tabWidth: 100 })).toBe(200 - pad);
|
||||
});
|
||||
|
||||
it('leaves an already-visible tab alone (callers skip the write)', () => {
|
||||
const helper = loadTabOverflowHelper();
|
||||
expect(helper.computeTabScrollLeft({ ...strip, scrollLeft: 100, tabLeft: 152, tabWidth: 100 })).toBe(100);
|
||||
});
|
||||
|
||||
it('never scrolls a strip that fits, and never leaves the scrollable range', () => {
|
||||
const helper = loadTabOverflowHelper();
|
||||
// Everything fits: nothing to scroll, whatever the tab geometry says.
|
||||
expect(
|
||||
helper.computeTabScrollLeft({ clientWidth: 900, scrollWidth: 400, scrollLeft: 0, tabLeft: 300, tabWidth: 100 })
|
||||
).toBe(0);
|
||||
// Clamped at both ends.
|
||||
const low = helper.computeTabScrollLeft({ ...strip, scrollLeft: 40, tabLeft: 4, tabWidth: 100 });
|
||||
expect(low).toBe(0);
|
||||
const high = helper.computeTabScrollLeft({ ...strip, scrollLeft: 0, tabLeft: 500, tabWidth: 58 });
|
||||
expect(high).toBeLessThanOrEqual(strip.scrollWidth - strip.clientWidth);
|
||||
});
|
||||
|
||||
it('aligns the start of a tab too wide to fit the window', () => {
|
||||
const helper = loadTabOverflowHelper();
|
||||
// 330px tab in a 335px window: no position shows it plus padding.
|
||||
expect(
|
||||
helper.computeTabScrollLeft({ clientWidth: 335, scrollWidth: 900, scrollLeft: 0, tabLeft: 400, tabWidth: 330 })
|
||||
).toBe(400);
|
||||
});
|
||||
|
||||
it('tolerates missing measurements instead of producing NaN', () => {
|
||||
const helper = loadTabOverflowHelper();
|
||||
expect(helper.computeTabScrollLeft({})).toBe(0);
|
||||
expect(helper.computeTabScrollLeft(undefined as unknown as ScrollInput)).toBe(0);
|
||||
expect(helper.TAB_SCROLL_REVEAL_PX).toBe(pad);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user