Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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
26 changes: 26 additions & 0 deletions packages/cli/src/ui/AppContainer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -987,6 +987,17 @@ export const AppContainer = (props: AppContainerProps) => {
// is swapped in once the real callback exists.
const openRewindSelectorRef = useRef<() => void>(() => {});

// /diff opens a per-turn diff dialog. Unlike rewind, no double-press or
// history-bound guard is needed, so the open/close handlers can live here
// (no ref bridge required).
const [isDiffDialogOpen, setIsDiffDialogOpen] = useState(false);
Comment thread
BZ-D marked this conversation as resolved.
const openDiffDialog = useCallback(() => {
setIsDiffDialogOpen(true);
}, []);
const closeDiffDialog = useCallback(() => {
setIsDiffDialogOpen(false);
}, []);

const slashCommandActions = useMemo(
() => ({
openAuthDialog,
Expand Down Expand Up @@ -1018,6 +1029,7 @@ export const AppContainer = (props: AppContainerProps) => {
openHooksDialog,
openResumeDialog,
openRewindSelector: () => openRewindSelectorRef.current(),
openDiffDialog,
handleResume,
handleBranch,
openDeleteDialog,
Expand Down Expand Up @@ -1049,6 +1061,7 @@ export const AppContainer = (props: AppContainerProps) => {
handleBranch,
openDeleteDialog,
openHelpDialog,
openDiffDialog,
],
);

Expand Down Expand Up @@ -2094,6 +2107,7 @@ export const AppContainer = (props: AppContainerProps) => {
isHelpDialogOpen ||
isExtensionsManagerDialogOpen ||
isRewindSelectorOpen ||
isDiffDialogOpen ||
bgTasksDialogOpen;
dialogsVisibleRef.current = dialogsVisible;
const shouldShowStickyTodos =
Expand Down Expand Up @@ -2562,6 +2576,8 @@ export const AppContainer = (props: AppContainerProps) => {
closeHelpDialog,
isBackgroundTasksDialogOpen: bgTasksDialogOpen,
closeBackgroundTasksDialog: closeBgTasksDialog,
isDiffDialogOpen,
closeDiffDialog,
});

const handleExit = useCallback(
Expand Down Expand Up @@ -3093,6 +3109,8 @@ export const AppContainer = (props: AppContainerProps) => {
// Rewind selector
isRewindSelectorOpen,
rewindEscPending,
// Diff dialog
isDiffDialogOpen,
}),
[
isThemeDialogOpen,
Expand Down Expand Up @@ -3215,6 +3233,8 @@ export const AppContainer = (props: AppContainerProps) => {
// Rewind selector
isRewindSelectorOpen,
rewindEscPending,
// Diff dialog
isDiffDialogOpen,
],
);

Expand Down Expand Up @@ -3294,6 +3314,9 @@ export const AppContainer = (props: AppContainerProps) => {
openRewindSelector,
closeRewindSelector,
handleRewindConfirm,
// Diff dialog
openDiffDialog,
closeDiffDialog,
}),
[
openThemeDialog,
Expand Down Expand Up @@ -3368,6 +3391,9 @@ export const AppContainer = (props: AppContainerProps) => {
openRewindSelector,
closeRewindSelector,
handleRewindConfirm,
// Diff dialog
openDiffDialog,
closeDiffDialog,
],
);

Expand Down
74 changes: 15 additions & 59 deletions packages/cli/src/ui/commands/diffCommand.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,12 @@ describe('diffCommand', () => {

it('errors when getWorkingDir and getProjectRoot both return empty', async () => {
if (!diffCommand.action) throw new Error('Command has no action');
// Non-interactive mode runs the fetchGitDiff path that actually needs a
// cwd. Interactive mode short-circuits to opening the dialog (the
// dialog's own hooks tolerate a missing cwd by showing the empty state),
// so the cwd guard only fires off the dialog path.
const noCwdContext = createMockCommandContext({
executionMode: 'non_interactive',
services: {
config: {
getWorkingDir: () => '',
Expand Down Expand Up @@ -301,75 +306,26 @@ describe('diffCommand interactive mode', () => {
mockFetchGitDiff = vi.mocked(fetchGitDiff);
});

it('dispatches a diff_stats history item instead of returning text', async () => {
it('opens the diff dialog without touching git or the history', async () => {
if (!diffCommand.action) throw new Error('Command has no action');
const ctx = makeInteractiveContext();
mockFetchGitDiff.mockResolvedValue({
stats: { filesCount: 2, linesAdded: 7, linesRemoved: 3 },
perFileStats: new Map([
['src/a.ts', { added: 5, removed: 2, isBinary: false }],
['src/b.ts', { added: 2, removed: 1, isBinary: false }],
]),
} satisfies GitDiffResult);

const result = await diffCommand.action(ctx, '');
expect(result).toBeUndefined();
expect(ctx.ui.addItem).toHaveBeenCalledTimes(1);
const call = (ctx.ui.addItem as Mock).mock.calls[0][0];
expect(call.type).toBe('diff_stats');
expect(call.model).toMatchObject({
filesCount: 2,
linesAdded: 7,
linesRemoved: 3,
hiddenCount: 0,
});
expect(call.model.rows).toHaveLength(2);
expect(call.model.rows[0]).toMatchObject({
filename: 'src/a.ts',
added: 5,
removed: 2,
isBinary: false,
isUntracked: false,
});
});

it('still returns a plain-text info message for the "clean tree" case', async () => {
if (!diffCommand.action) throw new Error('Command has no action');
const ctx = makeInteractiveContext();
mockFetchGitDiff.mockResolvedValue({
stats: { filesCount: 0, linesAdded: 0, linesRemoved: 0 },
perFileStats: new Map(),
} satisfies GitDiffResult);

const result = await diffCommand.action(ctx, '');
expect(result).toMatchObject({ type: 'message', messageType: 'info' });
expect(result).toEqual({ type: 'dialog', dialog: 'diff' });
// Dialog ownership: the data fetch happens inside the dialog's hooks,
// not in the command. Asserting we *don't* call git here keeps the
// contract from regressing to the old "summary in scroll history"
// behavior, which paid for a git fetch before the user could even
// see the picker.
expect(mockFetchGitDiff).not.toHaveBeenCalled();
expect(ctx.ui.addItem).not.toHaveBeenCalled();
});

it('still returns an error MessageActionReturn when fetchGitDiff throws', async () => {
it('errors when config is unavailable even in interactive mode', async () => {
if (!diffCommand.action) throw new Error('Command has no action');
const ctx = makeInteractiveContext();
mockFetchGitDiff.mockRejectedValueOnce(new Error('boom'));

const ctx = createMockCommandContext({ executionMode: 'interactive' });
const result = await diffCommand.action(ctx, '');
expect(result).toMatchObject({ type: 'message', messageType: 'error' });
expect(ctx.ui.addItem).not.toHaveBeenCalled();
});

it('propagates hiddenCount to the history item for fast-path results', async () => {
if (!diffCommand.action) throw new Error('Command has no action');
const ctx = makeInteractiveContext();
mockFetchGitDiff.mockResolvedValue({
stats: { filesCount: 60, linesAdded: 100, linesRemoved: 20 },
perFileStats: new Map([
['src/a.ts', { added: 1, removed: 0, isBinary: false }],
]),
} satisfies GitDiffResult);

await diffCommand.action(ctx, '');
const call = (ctx.ui.addItem as Mock).mock.calls[0][0];
expect(call.model.hiddenCount).toBe(59);
expect(call.model.rows).toHaveLength(1);
});
});

Expand Down
69 changes: 11 additions & 58 deletions packages/cli/src/ui/commands/diffCommand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,16 @@ import {
CommandKind,
type CommandContext,
type MessageActionReturn,
type OpenDialogActionReturn,
type SlashCommand,
} from './types.js';
import { t } from '../../i18n/index.js';
import {
MessageType,
type DiffRenderModel,
type DiffRenderRow,
type HistoryItemDiffStats,
} from '../types.js';
import { escapeAnsiCtrlCodes } from '../utils/textUtils.js';
import { type DiffRenderModel, type DiffRenderRow } from '../types.js';
import { sanitizeFilenameForDisplay } from '../utils/textUtils.js';

async function diffAction(
context: CommandContext,
): Promise<MessageActionReturn | void> {
): Promise<MessageActionReturn | OpenDialogActionReturn | void> {
const { config } = context.services;
if (!config) {
return {
Expand All @@ -36,6 +32,13 @@ async function diffAction(
};
}

// Interactive mode: open the per-turn diff dialog. Non-interactive / ACP
// paths keep the plain-text "working tree vs HEAD" summary so pipes, logs,
// and remote transports that don't speak Ink still get legible output.
if (context.executionMode === 'interactive') {
return { type: 'dialog', dialog: 'diff' };
}

const cwd = config.getWorkingDir() || config.getProjectRoot();
if (!cwd) {
return {
Expand Down Expand Up @@ -78,19 +81,6 @@ async function diffAction(

const model = buildDiffRenderModel(result);

// Interactive path: dispatch a structured history item so `DiffStatsDisplay`
// can render with theme colors. Non-interactive / ACP stay on the
// plain-text MessageActionReturn path so pipes, logs, and transports that
// don't speak Ink still see legible output.
if (context.executionMode === 'interactive') {
const item: HistoryItemDiffStats = {
type: MessageType.DIFF_STATS,
model,
};
context.ui.addItem(item, Date.now());
return;
}

return {
type: 'message',
messageType: 'info',
Expand Down Expand Up @@ -207,43 +197,6 @@ export function renderDiffModelText(model: DiffRenderModel): string {
return lines.length > 0 ? `${header}\n${lines.join('\n')}${capNote}` : header;
}

// Match standalone C0 controls (incl. TAB/CR/LF/BEL/BS), DEL, and C1 controls.
// `escapeAnsiCtrlCodes` only neutralizes multi-byte ANSI sequences, so a
// filename like `bad\nINJECTED.txt` or `bad\roverwrite.txt` would otherwise
// still break layout in the non-interactive / ACP rendering path.
// eslint-disable-next-line no-control-regex
const FILENAME_CONTROL_CHARS_REGEX = /[\x00-\x1f\x7f-\x9f]/g;

function escapeControlChar(ch: string): string {
switch (ch) {
case '\b':
return '\\b';
case '\t':
return '\\t';
case '\n':
return '\\n';
case '\f':
return '\\f';
case '\r':
return '\\r';
default: {
// JSON.stringify only escapes 0x00-0x1F (and `"` / `\`); DEL (0x7F) and
// C1 controls (0x80-0x9F) are returned as raw bytes, which is exactly
// what we are trying to keep out of the rendered output. Hand-roll the
// \uXXXX escape so every matched code point becomes printable.
const code = ch.charCodeAt(0);
return `\\u${code.toString(16).padStart(4, '0')}`;
}
}
}

function sanitizeFilenameForDisplay(name: string): string {
return escapeAnsiCtrlCodes(name).replace(
FILENAME_CONTROL_CHARS_REGEX,
escapeControlChar,
);
}

function formatRowsText(rows: DiffRenderRow[]): string[] {
if (rows.length === 0) return [];
const { addWidth, remWidth, statColumnWidth } = computeDiffColumnWidths(rows);
Expand Down
3 changes: 2 additions & 1 deletion packages/cli/src/ui/commands/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,8 @@ export interface OpenDialogActionReturn {
| 'extensions_manage'
| 'hooks'
| 'mcp'
| 'rewind';
| 'rewind'
| 'diff';
}

/**
Expand Down
13 changes: 13 additions & 0 deletions packages/cli/src/ui/components/DialogManager.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import { MCPManagementDialog } from './mcp/MCPManagementDialog.js';
import { HooksManagementDialog } from './hooks/HooksManagementDialog.js';
import { SessionPicker } from './SessionPicker.js';
import { RewindSelector } from './RewindSelector.js';
import { DiffDialog } from './DiffDialog.js';
import { MemoryDialog } from './MemoryDialog.js';
import { Help } from './Help.js';
import { BackgroundTasksDialog } from './background-view/BackgroundTasksDialog.js';
Expand Down Expand Up @@ -472,6 +473,18 @@ export const DialogManager = ({
);
}

if (uiState.isDiffDialogOpen) {
Comment thread
BZ-D marked this conversation as resolved.
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] No Error Boundary wraps DiffDialog. A render exception crashes the entire Ink TUI to shell with no visible stack trace.

— DeepSeek/deepseek-v4-pro via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Deferring — no ErrorBoundary exists anywhere in packages/cli/src today (grep -r componentDidCatch returns zero hits). Adding one for just this dialog would introduce a class-component pattern that doesn't exist in the otherwise pure-functional codebase, and the right answer is a uniform boundary at the DialogManager level — both out of scope for this PR.

The other correctness gates landed in f9667f2 (per-file try/catch in computeTurnFileDiff, swallow-on-fail in useTurnDiffs) substantially reduce the surface that would actually crash the dialog at runtime. I'll open a follow-up issue tagged scope/components for the global boundary.

return (
<DiffDialog
history={uiState.history}
cwd={config.getWorkingDir() || config.getProjectRoot()}
fileHistoryService={config.getFileHistoryService()}
fileCheckpointingEnabled={config.getFileCheckpointingEnabled()}
onClose={uiActions.closeDiffDialog}
/>
);
}

// Background tasks dialog — lowest priority so other dialogs
// (permissions, trust prompts, auth, etc.) always take precedence. The
// dialog is part of the shared dialogsVisible machinery (see
Expand Down
Loading
Loading