Code review comments as a pr (#21209)

Author: jacob314

packages/cli/src/ui/components/Footer.test.tsx

@@ -132,9 +132,7 @@ describe('<Footer />', () => {
       const output = lastFrame();
       expect(output).toBeDefined();
       // Should contain some part of the path, likely shortened
-      expect(output).toContain(
-        path.join('directories', 'to', 'make', 'it', 'long'),
-      );
+      expect(output).toContain(path.join('make', 'it'));
       unmount();
     });
 
@@ -149,9 +147,38 @@ describe('<Footer />', () => {
       await waitUntilReady();
       const output = lastFrame();
       expect(output).toBeDefined();
-      expect(output).toContain(
-        path.join('directories', 'to', 'make', 'it', 'long'),
+      expect(output).toContain(path.join('make', 'it'));
+      unmount();
+    });
+
+    it('should not truncate high-priority items on narrow terminals (regression)', async () => {
+      const { lastFrame, waitUntilReady, unmount } = renderWithProviders(
+        <Footer />,
+        {
+          width: 60,
+          uiState: {
+            sessionStats: mockSessionStats,
+          },
+          settings: createMockSettings({
+            general: {
+              vimMode: true,
+            },
+            ui: {
+              footer: {
+                showLabels: true,
+                items: ['workspace', 'model-name'],
+              },
+            },
+          }),
+        },
       );
+      await waitUntilReady();
+      const output = lastFrame();
+      // [INSERT] is high priority and should be fully visible
+      // (Note: VimModeProvider defaults to 'INSERT' mode when enabled)
+      expect(output).toContain('[INSERT]');
+      // Other items should be present but might be shortened
+      expect(output).toContain('gemini-pro');
       unmount();
     });
   });

packages/cli/src/ui/components/Footer.tsx

@@ -103,6 +103,9 @@ export interface FooterRowItem {
   key: string;
   header: string;
   element: React.ReactNode;
+  flexGrow?: number;
+  flexShrink?: number;
+  isFocused?: boolean;
 }
 
 const COLUMN_GAP = 3;
@@ -123,10 +126,20 @@ export const FooterRow: React.FC<{
     }
 
     elements.push(
-      <Box key={item.key} flexDirection="column">
+      <Box
+        key={item.key}
+        flexDirection="column"
+        flexGrow={item.flexGrow ?? 0}
+        flexShrink={item.flexShrink ?? 1}
+        backgroundColor={item.isFocused ? theme.background.focus : undefined}
+      >
         {showLabels && (
           <Box height={1}>
-            <Text color={theme.ui.comment}>{item.header}</Text>
+            <Text
+              color={item.isFocused ? theme.text.primary : theme.ui.comment}
+            >
+              {item.header}
+            </Text>
           </Box>
         )}
         <Box height={1}>{item.element}</Box>
@@ -138,6 +151,7 @@ export const FooterRow: React.FC<{
     <Box
       flexDirection="row"
       flexWrap="nowrap"
+      width="100%"
       columnGap={showLabels ? COLUMN_GAP : 0}
     >
       {elements}
@@ -408,41 +422,50 @@ export const Footer: React.FC = () => {
   }
 
   // --- Width Fitting Logic ---
-  let currentWidth = 2; // Initial padding
   const columnsToRender: FooterColumn[] = [];
   let droppedAny = false;
+  let currentUsedWidth = 2; // Initial padding
 
-  for (let i = 0; i < potentialColumns.length; i++) {
-    const col = potentialColumns[i];
-    const gap = columnsToRender.length > 0 ? (showLabels ? COLUMN_GAP : 3) : 0; // Use 3 for dot separator width
+  for (const col of potentialColumns) {
+    const gap = columnsToRender.length > 0 ? (showLabels ? COLUMN_GAP : 3) : 0;
     const budgetWidth = col.id === 'workspace' ? 20 : col.width;
 
     if (
       col.isHighPriority ||
-      currentWidth + gap + budgetWidth <= terminalWidth - 2
+      currentUsedWidth + gap + budgetWidth <= terminalWidth - 2
     ) {
       columnsToRender.push(col);
-      currentWidth += gap + budgetWidth;
+      currentUsedWidth += gap + budgetWidth;
     } else {
       droppedAny = true;
     }
   }
 
-  const totalBudgeted = columnsToRender.reduce(
-    (sum, c, idx) =>
-      sum +
-      (c.id === 'workspace' ? 20 : c.width) +
-      (idx > 0 ? (showLabels ? COLUMN_GAP : 3) : 0),
-    2,
-  );
-  const excessSpace = Math.max(0, terminalWidth - totalBudgeted);
-
   const rowItems: FooterRowItem[] = columnsToRender.map((col) => {
-    const maxWidth = col.id === 'workspace' ? 20 + excessSpace : col.width;
+    const isWorkspace = col.id === 'workspace';
+
+    // Calculate exact space available for growth to prevent over-estimation truncation
+    const otherItemsWidth = columnsToRender
+      .filter((c) => c.id !== 'workspace')
+      .reduce((sum, c) => sum + c.width, 0);
+    const numItems = columnsToRender.length + (droppedAny ? 1 : 0);
+    const numGaps = numItems > 1 ? numItems - 1 : 0;
+    const gapsWidth = numGaps * (showLabels ? COLUMN_GAP : 3);
+    const ellipsisWidth = droppedAny ? 1 : 0;
+
+    const availableForWorkspace = Math.max(
+      20,
+      terminalWidth - 2 - gapsWidth - otherItemsWidth - ellipsisWidth,
+    );
+
+    const estimatedWidth = isWorkspace ? availableForWorkspace : col.width;
+
     return {
       key: col.id,
       header: col.header,
-      element: col.element(maxWidth),
+      element: col.element(estimatedWidth),
+      flexGrow: isWorkspace ? 1 : 0,
+      flexShrink: isWorkspace ? 1 : 0,
     };
   });
 
@@ -451,6 +474,8 @@ export const Footer: React.FC = () => {
       key: 'ellipsis',
       header: '',
       element: <Text color={theme.ui.comment}>…</Text>,
+      flexGrow: 0,
+      flexShrink: 0,
     });
   }
 

packages/cli/src/ui/components/FooterConfigDialog.test.tsx

@@ -24,13 +24,14 @@ describe('<FooterConfigDialog />', () => {
 
   it('renders correctly with default settings', async () => {
     const settings = createMockSettings();
-    const { lastFrame, waitUntilReady } = renderWithProviders(
+    const renderResult = renderWithProviders(
       <FooterConfigDialog onClose={mockOnClose} />,
       { settings },
     );
 
-    await waitUntilReady();
-    expect(lastFrame()).toMatchSnapshot();
+    await renderResult.waitUntilReady();
+    expect(renderResult.lastFrame()).toMatchSnapshot();
+    await expect(renderResult).toMatchSvgSnapshot();
   });
 
   it('toggles an item when enter is pressed', async () => {
@@ -66,7 +67,7 @@ describe('<FooterConfigDialog />', () => {
     );
 
     await waitUntilReady();
-    // Initial order: workspace, branch, ...
+    // Initial order: workspace, git-branch, ...
     const output = lastFrame();
     const cwdIdx = output.indexOf('] workspace');
     const branchIdx = output.indexOf('] git-branch');
@@ -108,22 +109,40 @@ describe('<FooterConfigDialog />', () => {
 
   it('highlights the active item in the preview', async () => {
     const settings = createMockSettings();
-    const { lastFrame, stdin, waitUntilReady } = renderWithProviders(
+    const renderResult = renderWithProviders(
       <FooterConfigDialog onClose={mockOnClose} />,
       { settings },
     );
 
+    const { lastFrame, stdin, waitUntilReady } = renderResult;
+
     await waitUntilReady();
     expect(lastFrame()).toContain('~/project/path');
 
-    // Move focus down to 'git-branch'
+    // Move focus down to 'code-changes' (which has colored elements)
+    for (let i = 0; i < 8; i++) {
+      act(() => {
+        stdin.write('\u001b[B'); // Down arrow
+      });
+    }
+
+    await waitFor(() => {
+      // The selected indicator should be next to 'code-changes'
+      expect(lastFrame()).toMatch(/> \[ \] code-changes/);
+    });
+
+    // Toggle it on
     act(() => {
-      stdin.write('\u001b[B'); // Down arrow
+      stdin.write('\r');
     });
 
     await waitFor(() => {
-      expect(lastFrame()).toContain('main');
+      // It should now be checked and appear in the preview
+      expect(lastFrame()).toMatch(/> \[✓\] code-changes/);
+      expect(lastFrame()).toContain('+12 -4');
     });
+
+    await expect(renderResult).toMatchSvgSnapshot();
   });
 
   it('shows an empty preview when all items are deselected', async () => {
@@ -134,20 +153,64 @@ describe('<FooterConfigDialog />', () => {
     );
 
     await waitUntilReady();
-    for (let i = 0; i < 10; i++) {
+
+    // Default items are the first 5. We toggle them off.
+    for (let i = 0; i < 5; i++) {
+      act(() => {
+        stdin.write('\r'); // Toggle off
+      });
       act(() => {
-        stdin.write('\r'); // Toggle (deselect)
         stdin.write('\u001b[B'); // Down arrow
       });
     }
 
+    await waitFor(
+      () => {
+        const output = lastFrame();
+        expect(output).toContain('Preview:');
+        expect(output).not.toContain('~/project/path');
+        expect(output).not.toContain('docker');
+      },
+      { timeout: 2000 },
+    );
+  });
+
+  it('moves item correctly after trying to move up at the top', async () => {
+    const settings = createMockSettings();
+    const { lastFrame, stdin, waitUntilReady } = renderWithProviders(
+      <FooterConfigDialog onClose={mockOnClose} />,
+      { settings },
+    );
+    await waitUntilReady();
+
+    // Default initial items in mock settings are 'git-branch', 'workspace', ...
     await waitFor(() => {
       const output = lastFrame();
-      expect(output).toContain('Preview:');
-      expect(output).not.toContain('~/project/path');
-      expect(output).not.toContain('docker');
-      expect(output).not.toContain('gemini-2.5-pro');
-      expect(output).not.toContain('1.2k left');
+      expect(output).toContain('] git-branch');
+      expect(output).toContain('] workspace');
+    });
+
+    const output = lastFrame();
+    const branchIdx = output.indexOf('] git-branch');
+    const workspaceIdx = output.indexOf('] workspace');
+    expect(workspaceIdx).toBeLessThan(branchIdx);
+
+    // Try to move workspace up (left arrow) while it's at the top
+    act(() => {
+      stdin.write('\u001b[D'); // Left arrow
+    });
+
+    // Move workspace down (right arrow)
+    act(() => {
+      stdin.write('\u001b[C'); // Right arrow
+    });
+
+    await waitFor(() => {
+      const outputAfter = lastFrame();
+      const bIdxAfter = outputAfter.indexOf('] git-branch');
+      const wIdxAfter = outputAfter.indexOf('] workspace');
+      // workspace should now be after git-branch
+      expect(bIdxAfter).toBeLessThan(wIdxAfter);
     });
   });
 });