Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions packages/cli/src/ui/components/HistoryItemDisplay.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ vi.mock('../hooks/useMouseEvents.js', () => ({
useMouseEvents: vi.fn(),
}));

import { useMouseEvents } from '../hooks/useMouseEvents.js';

import { toggleKeyHint } from './messages/ConversationMessages.js';

describe('<HistoryItemDisplay />', () => {
Expand Down Expand Up @@ -539,4 +541,40 @@ describe('<HistoryItemDisplay />', () => {
expect(lastFrame()).not.toMatch(/\[\d{2}:\d{2}:\d{2}\]/);
});
});

describe('thinking-block mouse tracking is VP-gated (non-VP scroll fix)', () => {
// A collapsed thinking block arms a click-to-expand mouse handler. The
// VP-gating itself lives in useMouseEvents (covered by its own test); here
// we pin the contract that the thinking block subscribes WITHOUT
// `bypassVpGate`, i.e. it is subject to the gate — so in non-VP it never
// turns on SGR mouse tracking and native terminal scrollback survives.
// (Alt+T still expands the block in non-VP.)
const thoughtItem: HistoryItem = {
id: 1,
type: 'gemini_thought',
text: 'Inspecting the repository',
durationMs: 1200,
};

it('subscribes the click handler without bypassVpGate (stays VP-gated)', () => {
vi.mocked(useMouseEvents).mockClear();
renderWithProviders(
<CompactModeProvider
value={{ compactMode: false, compactInline: false }}
>
<HistoryItemDisplay
item={thoughtItem}
terminalWidth={100}
isPending={false}
/>
</CompactModeProvider>,
);
expect(vi.mocked(useMouseEvents)).toHaveBeenCalled();
const opts = vi.mocked(useMouseEvents).mock.calls.at(-1)?.[1];
// Collapsed thought → the handler is "active", but it must NOT bypass the
// VP gate, so useMouseEvents only arms it in VP mode.
expect(opts?.isActive).toBe(true);
expect(opts?.bypassVpGate ?? false).toBe(false);
});
});
});
4 changes: 4 additions & 0 deletions packages/cli/src/ui/components/HistoryItemDisplay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ const ClickableThinkMessage: React.FC<{
}) => {
const ref = useRef<DOMElement>(null);
const { openThinkingViewer } = useThinkingViewer();
// Click-to-expand needs SGR mouse tracking. We do NOT pass `bypassVpGate`, so
// useMouseEvents enables it only in VP mode; in non-VP the click handler
// stays dormant and native terminal scrollback is preserved (the block still
// expands via Alt+T — the "option+t to expand" affordance it already shows).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The comment says "Click-to-expand" and "the block still expands via Alt+T," but the click handler at line 134 calls openThinkingViewer(...) — it opens the modal viewer, not inline expansion. Alt+T (AppContainer.tsx:3290) calls setThoughtExpanded — it toggles inline expansion only. These are distinct features. After the VP gate, the ThinkingViewer modal becomes completely inaccessible in non-VP mode: no keyboard shortcut opens it, and the click handler is dormant. The comment's "the block still expands via Alt+T" is misleading because it implies the click's functionality (modal) is covered by Alt+T (inline), when they are different.

Suggested change
// expands via Alt+T — the "option+t to expand" affordance it already shows).
// Click-to-open-viewer needs SGR mouse tracking. We do NOT pass `bypassVpGate`, so
// useMouseEvents enables it only in VP mode; in non-VP the click handler
// stays dormant and native terminal scrollback is preserved. The block still
// expands inline via Alt+T, but the full-screen modal viewer is only
// accessible in VP mode (no keyboard shortcut opens it in non-VP).

— glm-5.2 via Qwen Code /review

const isActive = !isPending && !expanded;
const sanitizedViewerText = useMemo(
() => escapeAnsiCtrlCodes(viewerText),
Expand Down
54 changes: 54 additions & 0 deletions packages/cli/src/ui/components/ThinkingViewer.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/**
* @license
* Copyright 2025 Qwen Team
* SPDX-License-Identifier: Apache-2.0
*/

import { describe, it, expect, vi, beforeEach } from 'vitest';
import { render } from 'ink-testing-library';
import { ThinkingViewer } from './ThinkingViewer.js';
import { useMouseEvents } from './../hooks/useMouseEvents.js';

// The modal viewer owns the wheel for its own scrolling and renders on the
// alternate screen in non-VP mode (no native scrollback to protect), so it must
// subscribe WITH `bypassVpGate: true`. Mirror of the HistoryItemDisplay test
// that pins the OPPOSITE contract for the click-to-expand handler. The gating
// logic itself lives in useMouseEvents (covered by its own test); here we only
// pin the option ThinkingViewer passes.
vi.mock('./../hooks/useMouseEvents.js', () => ({
useMouseEvents: vi.fn(),
}));

// Avoid the real terminal-size / keypress / frame-flush wiring — they are not
// part of the contract under test and would otherwise require a TTY + context.
vi.mock('./../hooks/useTerminalSize.js', () => ({
useTerminalSize: () => ({ rows: 24, columns: 80 }),
}));
vi.mock('./../hooks/useKeypress.js', () => ({
useKeypress: vi.fn(),
}));
vi.mock('./../hooks/use-frame-coalesced-flush.js', () => ({
useFrameCoalescedFlush: () => ({ schedule: vi.fn(), cancel: vi.fn() }),
}));

describe('ThinkingViewer mouse tracking', () => {
beforeEach(() => {
vi.mocked(useMouseEvents).mockClear();
});

it('subscribes the wheel handler WITH bypassVpGate (works in non-VP)', () => {
render(
<ThinkingViewer
data={{ text: 'Inspecting the repository', durationMs: 1200 }}
onClose={() => {}}
useAlternateScreen={false}
/>,
);
expect(vi.mocked(useMouseEvents)).toHaveBeenCalled();
const opts = vi.mocked(useMouseEvents).mock.calls.at(-1)?.[1];
expect(opts?.isActive).toBe(true);
// Must bypass the VP gate, or wheel scrolling in the modal silently breaks
// in non-VP mode.
expect(opts?.bypassVpGate).toBe(true);
});
});
5 changes: 4 additions & 1 deletion packages/cli/src/ui/components/ThinkingViewer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,10 @@ export const ThinkingViewer: FC<ThinkingViewerProps> = ({
},
[scheduleWheelFlush],
),
{ isActive: true },
// Modal viewer renders on the alternate screen in non-VP mode (no native
Comment thread
chiga0 marked this conversation as resolved.
Resolved
// scrollback to protect), and owns the wheel for its own scrolling — opt
// out of the VP gate so it works in both VP and non-VP.
{ isActive: true, bypassVpGate: true },
);

const title =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,4 +260,107 @@ describe('<InlineParallelAgentsDisplay />', () => {
// Tally counts only the agent.
expect(frame).toContain('0/1 done');
});

Comment thread
chiga0 marked this conversation as resolved.
Resolved
describe('height backstop (availableTerminalHeight)', () => {
const manyAgents = (n: number): IndividualToolCallDisplay[] =>
Array.from({ length: n }, (_, i) =>
agentToolCall({
callId: `cap-${i}`,
subagentName: 'general-purpose',
taskDescription: `CapAgent ${i}`,
status: 'completed',
}),
);

const renderCapped = (
toolCalls: IndividualToolCallDisplay[],
availableTerminalHeight?: number,
) =>
render(
<ConfigContext.Provider value={undefined}>
<InlineParallelAgentsDisplay
toolCalls={toolCalls}
contentWidth={120}
availableTerminalHeight={availableTerminalHeight}
/>
</ConfigContext.Provider>,
);

it('without a budget, renders every agent (no cap)', () => {
const { lastFrame } = renderCapped(manyAgents(10));
const frame = lastFrame() ?? '';
// header + 10 rows, no overflow indicator.
expect(frame.split('\n').length).toBe(11);
expect(frame).not.toContain('more agent');
});

it('at the exact-fit budget, renders every agent (backstop is a no-op)', () => {
// Budget = rows.length + 1 = 11 pins the `>` boundary precisely:
// 11 > 11 = false → correct, no windowing (renders all 10 + header)
// 11 >= 11 = true → a regression to `>=` would fire windowing and emit
// a "+1 more" indicator even though everything fits.
// The `not.toContain('more agent')` assertion is what catches that flip
// (the line count alone coincides at 11 in both branches).
const { lastFrame } = renderCapped(manyAgents(10), 11);
const frame = lastFrame() ?? '';
expect(frame.split('\n').length).toBe(11);
expect(frame).not.toContain('more agent');
});

it('with a budget, windows to the most recent rows + "+N more" and never exceeds the budget', () => {
const budget = 6;
const { lastFrame } = renderCapped(manyAgents(10), budget);
const frame = lastFrame() ?? '';
Comment thread
chiga0 marked this conversation as resolved.
Resolved
// The whole non-Static frame must fit the budget, else ink clears the
// terminal every repaint (the scroll snap-back this guards against).
expect(frame.split('\n').length).toBeLessThanOrEqual(budget);
// rowBudget = budget - 2 = 4 visible rows → 6 hidden.
expect(frame).toContain('+6 more agents');
// Windows from the END (most recent), so the last agent shows and the
// first does not.
expect(frame).toContain('CapAgent 9');
expect(frame).not.toContain('CapAgent 0');
// Header tally still reflects the full count.
expect(frame).toContain('10/10 done');
});

it('at a budget of 3, shows exactly one (most-recent) row + indicator', () => {
// The transition point: budget 2 shows zero data rows, budget 3 shows the
// first one (rowsFit = budget - 2 = 1). Pins the off-by-one in
// `rowsFit = availableTerminalHeight - 2` and in `rows.slice(...)`.
const budget = 3;
const { lastFrame } = renderCapped(manyAgents(10), budget);
const frame = lastFrame() ?? '';
expect(frame.split('\n').length).toBe(budget);
expect(frame).toContain('+9 more agents');
// The single visible row is the most recent agent, not an older one.
expect(frame).toContain('CapAgent 9');
expect(frame).not.toContain('CapAgent 0');
expect(frame).toContain('10/10 done');
});

it('at a budget of 2, keeps the header + indicator only (drops every row)', () => {
const budget = 2;
const { lastFrame } = renderCapped(manyAgents(10), budget);
const frame = lastFrame() ?? '';
// header (1) + "+N more" indicator (1) = 2 rows, exactly the budget — no
// data row may slip in, or the frame overflows and snaps back.
expect(frame.split('\n').length).toBeLessThanOrEqual(budget);
expect(frame).toContain('+10 more agents');
expect(frame).not.toContain('CapAgent');
expect(frame).toContain('10/10 done');
});

it('at a budget of 1, keeps only the header (drops rows and the indicator)', () => {
const budget = 1;
const { lastFrame } = renderCapped(manyAgents(10), budget);
const frame = lastFrame() ?? '';
// Only the header fits; even the overflow indicator would overflow.
expect(frame.split('\n').length).toBeLessThanOrEqual(budget);
expect(frame).not.toContain('more agent');
expect(frame).not.toContain('CapAgent');
// The header label still carries the full tally.
expect(frame).toContain('10/10 done');
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,16 @@ interface InlineParallelAgentsDisplayProps {
* defaults to the number of agent entries in `toolCalls`.
*/
totalAgentCount?: number;
/**
* Hard cap on the panel's rendered height (rows). The panel renders
* inside the non-`<Static>` live frame; if that frame exceeds the
* terminal height, ink clears the whole screen on every repaint
* (scroll snap-back / flicker — see ink `shouldClearTerminalForFrame`).
* When set, the agent list windows to the most recent rows that fit,
* leaving a "+N more" indicator. Omitted → no cap (committed phase,
* where the row already lives in `<Static>`).
*/
availableTerminalHeight?: number;
}

/**
Expand Down Expand Up @@ -173,7 +183,12 @@ function truncateMiddle(input: string, max: number): string {

export const InlineParallelAgentsDisplay: React.FC<
InlineParallelAgentsDisplayProps
> = ({ toolCalls, contentWidth, totalAgentCount }) => {
> = ({
toolCalls,
contentWidth,
totalAgentCount,
availableTerminalHeight,
}) => {
const config = useContext(ConfigContext);

// Static slice of agent calls for this group. The caller already
Expand Down Expand Up @@ -263,20 +278,65 @@ export const InlineParallelAgentsDisplay: React.FC<
const total = totalAgentCount ?? rows.length;
const headerLabel = `Parallel agents · ${total} · ${doneCount}/${total} done`;

// Height backstop: the panel lives in the non-`<Static>` live frame, so its
// total height must stay within budget or ink clears the whole terminal on
// every repaint (scroll snap-back). The header always costs 1 row; when rows
// overflow, the "+N more" indicator costs another. Window to the most recent
// rows that still fit AFTER reserving those lines, so the rendered height
// (header + optional indicator + visibleRows) never exceeds the budget — even
// at degenerate budgets ≤ 2, where we drop all rows (and at a budget of 1 the
// indicator too, leaving just the header whose label still states the total).
// A budget ≤ 0 / undefined means "no cap" (committed phase — already in
// `<Static>`).
const hasBudget =
availableTerminalHeight != null && availableTerminalHeight > 0;
let overflowCount = 0;
let visibleRows = rows;
if (hasBudget && rows.length + 1 > availableTerminalHeight) {
// header (1) + all rows would exceed the budget → window.
if (availableTerminalHeight >= 2) {
// Reserve 1 row for the header and 1 for the "+N more" indicator; the
// remainder is for rows.
const rowsFit = availableTerminalHeight - 2;
Comment thread
chiga0 marked this conversation as resolved.
Resolved
overflowCount = rows.length - rowsFit;
visibleRows = rowsFit > 0 ? rows.slice(rows.length - rowsFit) : [];
} else {
// Budget of 1: only the header fits — drop every row and the indicator
// too. The header label still states the total agent count.
visibleRows = [];
}
}

return (
<Box flexDirection="column" width={contentWidth} paddingX={1}>
<Box>
<Text bold color={theme.text.accent}>
{headerLabel}
</Text>
</Box>
{rows.map((row) => (
{overflowCount > 0 && (
<Box>
<Text color={theme.text.secondary}>
Comment thread
chiga0 marked this conversation as resolved.
Resolved
… +{overflowCount} more {overflowCount === 1 ? 'agent' : 'agents'}
</Text>
</Box>
)}
{/* INVARIANT: each AgentRow must render exactly 1 terminal line. The
height backstop above (rowsFit = budget - 2) counts one line per row;
if AgentRow ever grows to multiple lines the frame would exceed the
budget and re-trigger the shouldClearTerminalForFrame snap-back this
guards against. Enforced by the wrap="truncate-end" Texts in AgentRow. */}
{visibleRows.map((row) => (
<AgentRow key={row.agentId} row={row} now={now} />
Comment thread
chiga0 marked this conversation as resolved.
Resolved
))}
</Box>
);
};

// INVARIANT: must render exactly ONE terminal line. The parent's height
// backstop (rowsFit = availableTerminalHeight - 2) assumes one line per row;
// all Text elements below use wrap="truncate-end" to hold that. Do not add
// wrapping/multi-line content here without revisiting that windowing math.
const AgentRow: React.FC<{ row: RowData; now: number }> = ({ row, now }) => {
const { glyph, color } = statusGlyph(row.status);
const safeName = escapeAnsiCtrlCodes(row.name);
Expand Down
Loading
Loading