fix(ui): address Jacob's code review comments and final UI cleanup

This commit is contained in:
Keith Guerin
2026-03-12 17:38:54 -07:00
parent c29668f08c
commit 34d3ea1abc
5 changed files with 105 additions and 39 deletions

View File

@@ -310,22 +310,16 @@ export const Composer = ({ isFocused = true }: { isFocused?: boolean }) => {
const hookIcon = activeHook?.eventName?.startsWith('After') ? '↩' : '↪';
return (
<Box flexDirection="row" alignItems="center">
<Box marginRight={1}>
<GeminiRespondingSpinner
nonRespondingDisplay={hookIcon}
isHookActive={true}
/>
</Box>
<Text color={theme.text.primary} italic wrap="truncate-end">
<HookStatusDisplay activeHooks={userHooks} />
</Text>
<Box flexDirection="row" alignItems="center" columnGap={1}>
<GeminiRespondingSpinner
nonRespondingDisplay={hookIcon}
isHookActive={true}
/>
<HookStatusDisplay activeHooks={userHooks} />
{showWit && uiState.currentWittyPhrase && (
<Box marginLeft={1}>
<Text color={theme.text.secondary} dimColor italic>
{uiState.currentWittyPhrase} :)
</Text>
</Box>
<Text color={theme.text.secondary} dimColor italic>
{uiState.currentWittyPhrase}
</Text>
)}
</Box>
);
@@ -353,7 +347,7 @@ export const Composer = ({ isFocused = true }: { isFocused?: boolean }) => {
* Renders the minimal metadata row content shown when UI details are hidden.
*/
const renderMinimalMetaRowContent = () => (
<Box flexDirection="row">
<Box flexDirection="row" columnGap={1}>
{showMinimalInlineLoading && (
<LoadingIndicator
inline
@@ -364,33 +358,16 @@ export const Composer = ({ isFocused = true }: { isFocused?: boolean }) => {
/>
)}
{hasUserHooks && (
<Box marginLeft={showMinimalInlineLoading ? 1 : 0}>
<Box marginRight={1}>
<GeminiRespondingSpinner isHookActive={true} />
</Box>
<Text color={theme.text.primary} italic>
<HookStatusDisplay activeHooks={userHooks} />
</Text>
<Box flexDirection="row" columnGap={1}>
<GeminiRespondingSpinner isHookActive={true} />
<HookStatusDisplay activeHooks={userHooks} />
</Box>
)}
{showMinimalBleedThroughRow && (
<Box marginLeft={showMinimalInlineLoading || hasUserHooks ? 1 : 0}>
<Box>
{miniMode_ShowApprovalMode && modeContentObj && (
<Text color={modeContentObj.color}> {modeContentObj.text}</Text>
)}
{/* {zenMode_ShowToast && (
<Box
marginLeft={
showMinimalInlineLoading ||
zenMode_ShowApprovalMode ||
hasUserHooks
? 1
: 0
}
>
<ToastDisplay />
</Box>
)} */}
</Box>
)}
</Box>

View File

@@ -78,4 +78,14 @@ describe('<HookStatusDisplay />', () => {
expect(lastFrame()).toContain('Working...');
unmount();
});
it('matches SVG snapshot for single hook', async () => {
const props = {
activeHooks: [{ name: 'test-hook', eventName: 'BeforeAgent', source: 'user' }],
};
const renderResult = render(<HookStatusDisplay {...props} />);
await renderResult.waitUntilReady();
await expect(renderResult).toMatchSvgSnapshot();
renderResult.unmount();
});
});

View File

@@ -8,6 +8,7 @@ import type React from 'react';
import { Text } from 'ink';
import { type ActiveHook } from '../types.js';
import { GENERIC_WORKING_LABEL } from '../textConstants.js';
import { theme } from '../semantic-colors.js';
interface HookStatusDisplayProps {
activeHooks: ActiveHook[];
@@ -38,9 +39,17 @@ export const HookStatusDisplay: React.FC<HookStatusDisplayProps> = ({
});
const text = `${label}: ${displayNames.join(', ')}`;
return <Text color="inherit">{text}</Text>;
return (
<Text color={theme.text.secondary} italic={true}>
{text}
</Text>
);
}
// If only system/extension hooks are running, show a generic message.
return <Text color="inherit">{GENERIC_WORKING_LABEL}</Text>;
return (
<Text color={theme.text.secondary} italic={true}>
{GENERIC_WORKING_LABEL}
</Text>
);
};

View File

@@ -1,5 +1,7 @@
// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
exports[`<HookStatusDisplay /> > matches SVG snapshot for single hook 1`] = `"Executing Hook: test-hook"`;
exports[`<HookStatusDisplay /> > should render a single executing hook 1`] = `
"Executing Hook: test-hook
"

View File

@@ -211,4 +211,72 @@ describe('useLoadingIndicator', () => {
expect(result.current.elapsedTime).toBe(0);
expect(result.current.currentLoadingPhrase).toBeUndefined();
});
it('should reflect retry status in currentLoadingPhrase when provided', () => {
const retryStatus = {
model: 'gemini-pro',
attempt: 2,
maxAttempts: 3,
delayMs: 1000,
};
const { result } = renderLoadingIndicatorHook(
StreamingState.Responding,
false,
retryStatus,
);
expect(result.current.currentLoadingPhrase).toContain('Trying to reach');
expect(result.current.currentLoadingPhrase).toContain('Attempt 3/3');
});
it('should hide low-verbosity retry status for early retry attempts', () => {
const retryStatus = {
model: 'gemini-pro',
attempt: 1,
maxAttempts: 5,
delayMs: 1000,
};
const { result } = renderLoadingIndicatorHook(
StreamingState.Responding,
false,
retryStatus,
'all',
'low',
);
expect(result.current.currentLoadingPhrase).not.toBe(
"This is taking a bit longer, we're still on it.",
);
});
it('should show a generic retry phrase in low error verbosity mode for later retries', () => {
const retryStatus = {
model: 'gemini-pro',
attempt: 2,
maxAttempts: 5,
delayMs: 1000,
};
const { result } = renderLoadingIndicatorHook(
StreamingState.Responding,
false,
retryStatus,
'all',
'low',
);
expect(result.current.currentLoadingPhrase).toBe(
"This is taking a bit longer, we're still on it.",
);
});
it('should show no phrases when loadingPhrasesMode is "off"', () => {
const { result } = renderLoadingIndicatorHook(
StreamingState.Responding,
false,
null,
'off',
);
expect(result.current.currentLoadingPhrase).toBeUndefined();
});
});