diff --git a/.changelog/next/changed-issue-4144.md b/.changelog/next/changed-issue-4144.md new file mode 100644 index 0000000000..f37bdc1cb6 --- /dev/null +++ b/.changelog/next/changed-issue-4144.md @@ -0,0 +1 @@ +- Chief of Staff now paints a dimension-reserving two-pane loading skeleton instead of a centered spinner, via a new PageSkeleton `layout="split"` mode diff --git a/client/src/components/ui/PageSkeleton.jsx b/client/src/components/ui/PageSkeleton.jsx index 8b2a93a0db..deee88a45e 100644 --- a/client/src/components/ui/PageSkeleton.jsx +++ b/client/src/components/ui/PageSkeleton.jsx @@ -16,6 +16,13 @@ // body region is loading (Goals). // layout 'stack' — vertically stacked cards, optional right sidebar. // 'grid' — responsive card grid (dashboard widgets, tiles). +// 'split' — two-pane shell: a fixed-width side rail beside a +// flexible main pane that owns the scroll (Chief of +// Staff). Unlike `stack`/`grid` this owns the whole +// shell, so it ignores `header` (a two-pane page's +// title lives inside a pane) and renders the tab strip +// INSIDE the main pane where a two-pane page puts it, +// not above the split. Tuned by `split*`/`side*` below. // tabs n > 0 — reserves a `TabPills` (underline variant) strip under // the header. A default-size TabPills button is `text-sm` // (20px line box) + `py-3`, i.e. 44px — its @@ -53,6 +60,25 @@ export default function PageSkeleton({ fullHeight = false, barClassName = 'px-3 py-2 sm:px-4 sm:py-3', bodyClassName = 'p-3 sm:p-4', + // `layout="split"` only. Mirror whatever the loaded two-pane shell does: + // sideCollapsed — the rail is collapsed on desktop: reserve a + // zero-width track there and keep the rail's mobile + // band, which a collapsible page still renders. + // splitColsClass — the desktop grid tracks. Defaults off + // `sideCollapsed` so a collapsed rail can't reserve a + // 320px column of nothing; pass your own when the + // page's rail isn't 320px wide. + // sideClassName — the rail's own box (borders, padding, scroll). + // sideHero — reserve a large circular block (avatar, portrait) + // at the top of the rail. + // sideBlocks — small blocks under the hero: a nav list at + // `grid-cols-1`, a stat grid at `grid-cols-2`. + sideCollapsed = false, + splitColsClass = sideCollapsed ? 'lg:grid-cols-[0px_1fr]' : 'lg:grid-cols-[320px_1fr]', + sideClassName = 'flex flex-col gap-3 border-b lg:border-b-0 lg:border-r border-port-border p-3 lg:p-4 lg:h-full lg:overflow-hidden', + sideHero = false, + sideBlocks = 4, + sideBlockColsClass = 'grid-cols-1', // Flex shape of the title/action row. The defaults are the common cases // (PageHeader's wrapping row for `bar`, stack-then-row for `inline`); pages // that break at a different width — or never stack at all — pass their own, @@ -79,11 +105,74 @@ export default function PageSkeleton({ )); + const stackedCards =
{cardBlocks}
; + + const tabRows = repeat(tabs); + const sideBlockRows = repeat(sideBlocks); + const renderTabStrip = (bordered) => tabRows.length > 0 ? ( +
+ {tabRows.map((_, i) => ( +
+
+
+ ))} +
+ ) : null; + + // `split` pages are the two-pane shells. The rail stacks above the main pane + // below `lg` (same as the loaded page), so the mobile band is reserved too. + if (layout === 'split') { + const sideRail = ( + <> +
+ {sideHero && ( +
+ )} + {sideBlockRows.length > 0 && ( +
+ {sideBlockRows.map((_, i) => ( +
+ ))} +
+ )} + + ); + + return ( +
+ {sideCollapsed ? ( + <> + {/* Desktop: the rail is collapsed to a zero-width track, but the + track still has to exist or the main pane lands in column 1. */} +
+
{sideRail}
+ + ) : ( +
{sideRail}
+ )} + {/* The main pane owns the scroll on a `fullHeight` split, same as the + loaded page — the root is `overflow-hidden`, so without this the + reserved cards are clipped instead of scrolling. */} +
+ {tabRows.length > 0 &&
{renderTabStrip(true)}
} + {stackedCards} +
+
+ ); + } + + // Built after the `split` return so a two-pane page doesn't allocate the + // stack/grid tree (and its sidebar card) it never renders. const body = layout === 'grid' ?
{cardBlocks}
: (
-
{cardBlocks}
+ {stackedCards} {sidebar && (
@@ -97,17 +186,6 @@ export default function PageSkeleton({
); - const tabRows = repeat(tabs); - const renderTabStrip = (bordered) => tabRows.length > 0 ? ( -
- {tabRows.map((_, i) => ( -
-
-
- ))} -
- ) : null; - // `bar` pages are the flex-column shells: the header bar and tab strip are // full-bleed, only the body region takes padding and owns the scroll. if (header === 'bar') { diff --git a/client/src/components/ui/PageSkeleton.test.jsx b/client/src/components/ui/PageSkeleton.test.jsx index 1e3a958e90..46d9824d10 100644 --- a/client/src/components/ui/PageSkeleton.test.jsx +++ b/client/src/components/ui/PageSkeleton.test.jsx @@ -159,4 +159,88 @@ describe('PageSkeleton', () => { const infinite = render(); expect(cardCount(infinite.container)).toBe(64); }); + + describe('layout="split"', () => { + const sideBlocks = (container) => container.querySelectorAll('.h-\\[52px\\]'); + + it('reserves the two-pane grid tracks and drops the stack sidebar', () => { + const { container } = render(); + expect(status().className).toContain('lg:grid'); + expect(status().className).toContain('lg:grid-cols-[320px_1fr]'); + // The stack layout's right sidebar must not sneak into the main pane. + expect(container.innerHTML).not.toContain('lg:grid-cols-[1fr_360px]'); + expect(cardCount(container)).toBe(2); + }); + + it('collapses the rail track by default when sideCollapsed, without a caller override', () => { + // Otherwise a collapsed rail reserves 320px of nothing on desktop. + render(); + expect(status().className).toContain('lg:grid-cols-[0px_1fr]'); + expect(status().className).not.toContain('lg:grid-cols-[320px_1fr]'); + }); + + it('takes caller-supplied grid tracks over the derived default', () => { + render(); + expect(status().className).toContain('lg:grid-cols-[240px_1fr]'); + }); + + it('renders the tab strip INSIDE the main pane, not above the split', () => { + const { container } = render(); + const strip = container.querySelector('.h-\\[44px\\]').closest('.flex-1'); + // The strip's pane is the main pane — the one holding the cards. + expect(strip).not.toBeNull(); + expect(strip.querySelectorAll('.rounded-lg.border.border-port-border.bg-port-card').length) + .toBeGreaterThan(0); + }); + + it('reserves the hero block and the rail blocks in the requested column count', () => { + const { container } = render( + + ); + expect(container.querySelector('.rounded-full')).not.toBeNull(); + expect(sideBlocks(container)).toHaveLength(4); + expect(container.innerHTML).toContain('grid gap-1.5 grid-cols-2'); + }); + + it('omits the hero and the rail blocks when they are not requested', () => { + const { container } = render(); + expect(container.querySelector('.rounded-full')).toBeNull(); + expect(sideBlocks(container)).toHaveLength(0); + }); + + it('keeps a zero-width desktop track plus the mobile band when sideCollapsed', () => { + const { container } = render(); + // The empty desktop track keeps the main pane in grid column 2. + expect(container.querySelector('.hidden.lg\\:block')).not.toBeNull(); + // The rail itself only survives below `lg`, where the page stacks. + const rail = sideBlocks(container)[0].closest('.lg\\:hidden'); + expect(rail).not.toBeNull(); + }); + + it('owns the full height only when fullHeight is set', () => { + const tall = render(); + expect(status().className).toContain('h-full'); + tall.unmount(); + + render(); + expect(status().className).not.toContain('h-full'); + }); + + it('gives the main pane the scroll on a fullHeight split, since the root hides overflow', () => { + const tall = render(); + expect(tall.container.querySelector('.flex-1').className).toContain('overflow-y-auto'); + tall.unmount(); + + // Without fullHeight the shell doesn't own a viewport, so nothing scrolls. + const short = render(); + expect(short.container.querySelector('.flex-1').className).not.toContain('overflow-y-auto'); + }); + + it('pads the main pane with bodyClassName only when padded', () => { + const { container } = render( + + ); + expect(container.querySelector('.flex-1').className).toContain('p-3 lg:p-4'); + }); + }); }); diff --git a/client/src/pages/ChiefOfStaff.jsx b/client/src/pages/ChiefOfStaff.jsx index 1e9e0a2773..b68201ccd8 100644 --- a/client/src/pages/ChiefOfStaff.jsx +++ b/client/src/pages/ChiefOfStaff.jsx @@ -9,6 +9,7 @@ import { Play, Pause, Square, Clock, CheckCircle, AlertCircle, Cpu, ChevronDown, import toast from '../components/ui/Toast'; import BrailleSpinner from '../components/BrailleSpinner'; import TabPills from '../components/ui/TabPills'; +import PageSkeleton from '../components/ui/PageSkeleton'; // Import from modular components import { @@ -637,10 +638,24 @@ export default function ChiefOfStaff() { }; if (loading) { + // Reserve the loaded two-pane shell (#4144) — `/cos` is an `isFullWidth` + // route, so a centered spinner reserved nothing and the whole page jumped + // into place on first paint. `desktopPanelCollapsed` comes from + // localStorage, so the skeleton already knows which rail width to hold. return ( -
- -
+ ); } diff --git a/client/src/pages/ChiefOfStaff.test.jsx b/client/src/pages/ChiefOfStaff.test.jsx index 13e0cb924f..2d18433cfb 100644 --- a/client/src/pages/ChiefOfStaff.test.jsx +++ b/client/src/pages/ChiefOfStaff.test.jsx @@ -85,6 +85,27 @@ const renderConfigTab = () => render( , ); +// #4144 — `/cos` is an `isFullWidth` route, so its `
` is a bare +// `relative overflow-hidden`. The old centered `h-64` BrailleSpinner reserved +// none of the loaded two-pane shell, and the whole page jumped into place on +// first paint. jsdom does no layout, so the guard pins the structure that +// reserves those dimensions: the busy region IS the two-pane grid. +describe('ChiefOfStaff loading skeleton', () => { + it('reserves the two-pane shell instead of a centered spinner while loading', async () => { + // Hold the first fetch open so the loading branch is what renders. + api.getCosStatus.mockReturnValue(new Promise(() => {})); + const { container } = renderConfigTab(); + + const busy = await screen.findByRole('status'); + expect(busy).toHaveAttribute('aria-busy', 'true'); + expect(busy).toHaveAttribute('aria-label', 'Loading Chief of Staff'); + expect(busy.className).toContain('lg:grid-cols-[320px_1fr]'); + expect(busy.className).toContain('h-full'); + // The old spinner shell — a fixed 16rem box centering its child. + expect(container.querySelector('.h-64')).toBeNull(); + }); +}); + describe('ChiefOfStaff handleForceEvaluate', () => { it('does not toast success or advance the status message when the evaluate fails', async () => { api.forceCosEvaluate.mockRejectedValue(new Error('evaluate failed'));