From 239fa7141e2d3cee9fc66197813aa1933b59dacb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=8B=E7=AB=9F?= <1048927295@qq.com> Date: Mon, 27 Apr 2026 11:39:19 +0800 Subject: [PATCH] refactor(cli): factor /diff column widths into a shared helper Audit of the colorize commit found one real DRY hazard: DiffStatsDisplay and renderDiffModelText each independently re-derived addWidth / remWidth / statColumnWidth from the same row list. If anyone later changed one formula, the interactive Ink output and the non-interactive plain text would silently fall out of column alignment. Extract the computation into computeDiffColumnWidths() exported from diffCommand.ts; both renderers now call it. Adds a focused unit test of the contract (empty rows, widest non-binary row wins, binary rows are ignored, untracked text rows count). Drop a redundant `Omit` annotation since the type already has no id field. --- .../cli/src/ui/commands/diffCommand.test.ts | 79 ++++++++++++++++++- packages/cli/src/ui/commands/diffCommand.ts | 45 ++++++++--- .../components/messages/DiffStatsDisplay.tsx | 18 ++--- scripts/unused-keys-only-in-locales.json | 2 +- 4 files changed, 118 insertions(+), 26 deletions(-) diff --git a/packages/cli/src/ui/commands/diffCommand.test.ts b/packages/cli/src/ui/commands/diffCommand.test.ts index cdab8c8c0..ca267b067 100644 --- a/packages/cli/src/ui/commands/diffCommand.test.ts +++ b/packages/cli/src/ui/commands/diffCommand.test.ts @@ -6,7 +6,7 @@ import type { Mock } from 'vitest'; import { vi, describe, it, expect, beforeEach } from 'vitest'; -import { diffCommand } from './diffCommand.js'; +import { computeDiffColumnWidths, diffCommand } from './diffCommand.js'; import { type CommandContext } from './types.js'; import { createMockCommandContext } from '../../test-utils/mockCommandContext.js'; import { fetchGitDiff, type GitDiffResult } from '@qwen-code/qwen-code-core'; @@ -340,6 +340,83 @@ describe('diffCommand interactive mode', () => { }); }); +describe('computeDiffColumnWidths', () => { + // Direct contract test — both the Ink component and the plain-text + // renderer call this helper, so its output binds their column alignment. + // If anyone changes the formula, both paths must shift together. + + it('reports min widths of 1 for an empty row list', () => { + expect(computeDiffColumnWidths([])).toEqual({ + addWidth: 1, + remWidth: 1, + statColumnWidth: 5, // `+_ -_` with single-digit padding + }); + }); + + it('sizes columns to the widest non-binary row', () => { + const widths = computeDiffColumnWidths([ + { + filename: 'a', + added: 9999, + removed: 5, + isBinary: false, + isUntracked: false, + truncated: false, + }, + { + filename: 'b', + added: 2, + removed: 100, + isBinary: false, + isUntracked: false, + truncated: false, + }, + ]); + // 1 (`+`) + 4 (digits) + 1 (` `) + 1 (`-`) + 3 (digits) = 10 + expect(widths).toEqual({ addWidth: 4, remWidth: 3, statColumnWidth: 10 }); + }); + + it('ignores binary rows when computing widths', () => { + // A binary row must not push the numeric column wider, otherwise the + // `~` placeholder ends up padded to a column that no real number ever + // occupies. + const widths = computeDiffColumnWidths([ + { + filename: 'a', + added: 1, + removed: 1, + isBinary: false, + isUntracked: false, + truncated: false, + }, + { + filename: 'b.bin', + isBinary: true, + isUntracked: false, + truncated: false, + }, + ]); + expect(widths).toEqual({ addWidth: 1, remWidth: 1, statColumnWidth: 5 }); + }); + + it('counts untracked text rows in width calculation', () => { + // Untracked rows render as `+N -0 filename (new)`; their `added` + // value must be allowed to widen the column. + const widths = computeDiffColumnWidths([ + { + filename: 'fresh.log', + added: 12345, + removed: 0, + isBinary: false, + isUntracked: true, + truncated: false, + }, + ]); + expect(widths.addWidth).toBe(5); + expect(widths.statColumnWidth).toBe(1 + 5 + 1 + 1 + 1); + }); +}); + describe('diffCommand registration', () => { it('declares all execution modes so it works in non-interactive and ACP', () => { expect(diffCommand.supportedModes).toEqual([ diff --git a/packages/cli/src/ui/commands/diffCommand.ts b/packages/cli/src/ui/commands/diffCommand.ts index c7337369a..4d09dc1c8 100644 --- a/packages/cli/src/ui/commands/diffCommand.ts +++ b/packages/cli/src/ui/commands/diffCommand.ts @@ -82,7 +82,7 @@ async function diffAction( // plain-text MessageActionReturn path so pipes, logs, and transports that // don't speak Ink still see legible output. if (context.executionMode === 'interactive') { - const item: Omit = { + const item: HistoryItemDiffStats = { type: MessageType.DIFF_STATS, model, }; @@ -136,6 +136,38 @@ function toRow(filename: string, s: PerFileStats): DiffRenderRow { }; } +/** + * Single source of truth for the per-row column layout. Used by both the + * Ink component and the plain-text renderer so the two paths can never + * silently disagree on alignment. + */ +export interface DiffColumnWidths { + /** Digits in the widest non-binary `added` value (min 1). */ + addWidth: number; + /** Digits in the widest non-binary `removed` value (min 1). */ + remWidth: number; + /** Visual width of the `+X -Y` stat column, used to pad the binary `~` + * marker so it lines up with the numeric rows. */ + statColumnWidth: number; +} + +export function computeDiffColumnWidths( + rows: readonly DiffRenderRow[], +): DiffColumnWidths { + let maxAdded = 0; + let maxRemoved = 0; + for (const r of rows) { + if (r.isBinary) continue; + if ((r.added ?? 0) > maxAdded) maxAdded = r.added ?? 0; + if ((r.removed ?? 0) > maxRemoved) maxRemoved = r.removed ?? 0; + } + const addWidth = String(maxAdded).length; + const remWidth = String(maxRemoved).length; + // `+` + addDigits + ' ' + `-` + remDigits. + const statColumnWidth = 1 + addWidth + 1 + 1 + remWidth; + return { addWidth, remWidth, statColumnWidth }; +} + /** * Plain-text rendering of a `DiffRenderModel`. Used in non-interactive / ACP * modes where no Ink renderer is available, and as the source of truth for @@ -168,16 +200,7 @@ export function renderDiffModelText(model: DiffRenderModel): string { function formatRowsText(rows: DiffRenderRow[]): string[] { if (rows.length === 0) return []; - let maxAdded = 0; - let maxRemoved = 0; - for (const r of rows) { - if (r.isBinary) continue; - if ((r.added ?? 0) > maxAdded) maxAdded = r.added ?? 0; - if ((r.removed ?? 0) > maxRemoved) maxRemoved = r.removed ?? 0; - } - const addWidth = String(maxAdded).length; - const remWidth = String(maxRemoved).length; - const statColumnWidth = 1 + addWidth + 1 + 1 + remWidth; + const { addWidth, remWidth, statColumnWidth } = computeDiffColumnWidths(rows); const out: string[] = []; for (const r of rows) { diff --git a/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx b/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx index caba4d374..8fc342f86 100644 --- a/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx +++ b/packages/cli/src/ui/components/messages/DiffStatsDisplay.tsx @@ -8,6 +8,7 @@ import type React from 'react'; import { Box, Text } from 'ink'; import { theme } from '../../semantic-colors.js'; import type { DiffRenderModel, DiffRenderRow } from '../../types.js'; +import { computeDiffColumnWidths } from '../../commands/diffCommand.js'; import { t } from '../../../i18n/index.js'; interface DiffStatsDisplayProps { @@ -24,19 +25,10 @@ export const DiffStatsDisplay: React.FC = ({ model, }) => { const { filesCount, linesAdded, linesRemoved, rows, hiddenCount } = model; - - // Reproduce the numeric-column alignment of the text renderer so the - // interactive and non-interactive outputs are visually interchangeable. - let maxAdded = 0; - let maxRemoved = 0; - for (const r of rows) { - if (r.isBinary) continue; - if ((r.added ?? 0) > maxAdded) maxAdded = r.added ?? 0; - if ((r.removed ?? 0) > maxRemoved) maxRemoved = r.removed ?? 0; - } - const addWidth = String(maxAdded).length; - const remWidth = String(maxRemoved).length; - const statColumnWidth = 1 + addWidth + 1 + 1 + remWidth; + // Single source of truth shared with `renderDiffModelText`, so the + // interactive Ink output and the non-interactive plain text never drift + // out of column alignment. + const { addWidth, remWidth, statColumnWidth } = computeDiffColumnWidths(rows); const headerLabel = filesCount === 1 diff --git a/scripts/unused-keys-only-in-locales.json b/scripts/unused-keys-only-in-locales.json index 72615d1df..9d599490e 100644 --- a/scripts/unused-keys-only-in-locales.json +++ b/scripts/unused-keys-only-in-locales.json @@ -1,5 +1,5 @@ { - "generatedAt": "2026-04-27T03:28:33.689Z", + "generatedAt": "2026-04-27T03:39:04.133Z", "keys": [ " Models: Qwen latest models\n", " qwen auth qwen-oauth - Authenticate with Qwen OAuth (discontinued)",