From 3a4a07736299dc2cfa002346dbc4eaf8fd519625 Mon Sep 17 00:00:00 2001 From: 2SinceJuly Date: Fri, 30 Oct 2026 23:40:51 +0800 Subject: [PATCH] fix(web-shell): keep "+ Add session" clickable when the catalog is empty (#15835) * fix(web-shell): keep + Add session clickable when the catalog is empty The add button was gated on the cached session catalog already containing an addable session, so the picker could never open to refresh itself in the one case its reload-on-open effect exists for. Gate the button on pane capacity alone and let the picker report an empty list after reloading. Closes #15678 * test(web-shell): pin the empty-picker guard with a negative assertion * test(web-shell): assert add-session button disables at MAX_PANES The PR narrowed canAdd to the capacity term only, and the new test pinned just the removed half (empty catalog stays enabled). Without a cap assertion, dropping the pane-count check would leave every test green while the picker could open a seventh pane. * test(web-shell): pin the add button stays enabled below MAX_PANES --------- Co-authored-by: 2SinceJuly <202121119+2SinceJuly@users.noreply.github.com> --- .../client/components/SplitView.module.css | 6 ++ .../client/components/SplitView.test.tsx | 69 +++++++++++++++++++ .../web-shell/client/components/SplitView.tsx | 12 +++- packages/web-shell/client/i18n.tsx | 2 + 4 files changed, 87 insertions(+), 2 deletions(-) diff --git a/packages/web-shell/client/components/SplitView.module.css b/packages/web-shell/client/components/SplitView.module.css index 5b876034983..6e980b732c2 100644 --- a/packages/web-shell/client/components/SplitView.module.css +++ b/packages/web-shell/client/components/SplitView.module.css @@ -110,6 +110,12 @@ cursor: pointer; } +.pickerEmpty { + padding: 7px 10px; + color: var(--muted-foreground); + font-size: 13px; +} + .pickerItemLabel { flex: 1 1 auto; min-width: 0; diff --git a/packages/web-shell/client/components/SplitView.test.tsx b/packages/web-shell/client/components/SplitView.test.tsx index f5af65c42f5..dfc6ace4255 100644 --- a/packages/web-shell/client/components/SplitView.test.tsx +++ b/packages/web-shell/client/components/SplitView.test.tsx @@ -10,6 +10,7 @@ import * as React from 'react'; import { act } from 'react'; import { createRoot, type Root } from 'react-dom/client'; import { I18nProvider } from '../i18n'; +import { MAX_SPLIT_PANES } from '../utils/splitUrl'; Object.assign(globalThis, { IS_REACT_ACT_ENVIRONMENT: true }); @@ -779,6 +780,30 @@ describe('SplitView', () => { render({ sessionIds: sessionsState.map((s) => s.sessionId) }); // Eight requested, but only six live panes mount. expect(panes()).toHaveLength(6); + // At the cap there is no room left, so the entry point is disabled. + const addButton = container!.querySelector( + 'button[aria-haspopup="listbox"]', + ) as HTMLButtonElement; + expect(addButton.disabled).toBe(true); + }); + + it('keeps the add button enabled one below MAX_PANES', () => { + sessionsState = Array.from({ length: MAX_SPLIT_PANES + 2 }, (_, i) => ({ + sessionId: `x${i}`, + workspaceCwd: '/w', + displayName: `Pane ${i}`, + })); + // One slot spare and a session left to add, so the entry point must work. + render({ + sessionIds: sessionsState + .slice(0, MAX_SPLIT_PANES - 1) + .map((session) => session.sessionId), + }); + expect(panes()).toHaveLength(MAX_SPLIT_PANES - 1); + const addButton = container!.querySelector( + 'button[aria-haspopup="listbox"]', + ) as HTMLButtonElement; + expect(addButton.disabled).toBe(false); }); it('isolates a crashing pane so the rest of the split survives', () => { @@ -1072,6 +1097,45 @@ describe('SplitView', () => { expect(hiddenSlots()).toHaveLength(0); }); + it('keeps the add button enabled when the catalog has no other sessions', () => { + sessionsState = [ + { sessionId: 's3', workspaceCwd: '/w', displayName: 'Three' }, + ]; + render({ sessionIds: ['s3'] }); + const addButton = container!.querySelector( + 'button[aria-haspopup="listbox"]', + ) as HTMLButtonElement; + // The cached catalog is not authoritative: another session may exist + // elsewhere, so an empty list must not disable the entry point. + expect(addButton.disabled).toBe(false); + openPicker(); + expect(pickerOptions()).toEqual([]); + expect(container!.querySelector('[role="listbox"]')?.textContent).toContain( + 'No other sessions', + ); + }); + + it('picks up a session created elsewhere after opening the empty picker', () => { + sessionsState = [ + { sessionId: 's3', workspaceCwd: '/w', displayName: 'Three' }, + ]; + render({ sessionIds: ['s3'] }); + openPicker(); + expect(pickerOptions()).toEqual([]); + // A session is created elsewhere while the picker was open. + sessionsState = [ + ...sessionsState, + { sessionId: 's6', workspaceCwd: '/w', displayName: 'Six' }, + ]; + act(() => + document.dispatchEvent( + new KeyboardEvent('keydown', { key: 'Escape', bubbles: true }), + ), + ); + openPicker(); + expect(pickerOptions()).toEqual(['Six']); + }); + it('reloads the session list when the picker opens (never a stale list)', () => { render({ sessionIds: ['s1'] }); // `useSessions` only fetches on mount; nothing reloads until the user acts. @@ -1104,6 +1168,11 @@ describe('SplitView', () => { ); openPicker(); expect(pickerOptions()).toEqual(['Two', 'Three', 'Four', 'Five']); + // The empty row is `role="presentation"`, so only a text read sees it: a + // populated picker must not keep advertising "No other sessions". + expect( + container!.querySelector('[role="listbox"]')?.textContent, + ).not.toContain('No other sessions'); }); it('mirrors the live pane set up to the parent as panes change', () => { diff --git a/packages/web-shell/client/components/SplitView.tsx b/packages/web-shell/client/components/SplitView.tsx index a5c52fb8df2..840ef2abfd2 100644 --- a/packages/web-shell/client/components/SplitView.tsx +++ b/packages/web-shell/client/components/SplitView.tsx @@ -471,7 +471,10 @@ export function SplitView({ () => allSessions.filter((session) => !paneIds.includes(session.sessionId)), [allSessions, paneIds], ); - const canAdd = paneIds.length < MAX_PANES && available.length > 0; + // The cached catalog may lag behind sessions created elsewhere, so the button + // must stay clickable whenever another pane fits. The picker reloads on open + // and reports an empty list itself. + const canAdd = paneIds.length < MAX_PANES; // Only offer per-pane maximize once there's another pane to maximize against — // a lone pane already fills the split. const canMaximize = paneIds.length > 1; @@ -532,8 +535,13 @@ export function SplitView({ > + {t('splitView.addPane')} - {pickerOpen && available.length > 0 && ( + {pickerOpen && (