feat(cli): per-turn /diff with interactive dialog#4277
Conversation
`/diff` now opens an interactive dialog in TUI mode with: - Current (working tree vs HEAD) plus one entry per past user turn - ←/→ to switch source, ↑/↓ to select a file, Enter for hunks, Esc to close - File list paginates at 8 entries, with new/deleted/untracked/binary tags Per-turn diffs are computed by FileHistoryService.getTurnDiff(promptId), which compares the snapshot at the start of that turn against the next snapshot (or the live worktree for the most recent turn). Files the snapshotter failed to capture are skipped rather than rendered against a stale predecessor. Non-interactive and ACP modes keep the existing plain-text summary so pipes, logs, and remote transports are unchanged.
📋 Review SummaryThis PR introduces a per-turn diff dialog for the interactive CLI, allowing users to inspect file changes from each conversation turn rather than only viewing the aggregate working-tree-vs-HEAD summary. The implementation is well-structured, with clean separation between core services and UI components, and includes comprehensive test coverage. The non-interactive and ACP paths remain unchanged, preserving backward compatibility for pipe consumers and remote transports. 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Two small audit-driven cleanups, no behavior change in normal sessions: - Match findSnapshot's last-occurrence-wins semantics so /rewind and /diff agree if a promptId is ever reused (defensive — promptIds are unique per submission in practice). - Drop the redundant `?? undefined` in the fast-path skip; `?.` already short-circuits to undefined, so the extra coalesce was noise.
Long absolute paths (~> 90 chars) previously overflowed the dialog and wrapped, shattering the file-list and detail-view alignment. Reserve a fixed budget for the tag/stats columns, head-truncate the path with a leading ellipsis so the basename — the part users actually read — is always visible. Also drop the dead MAX_FILES_FOR_DETAILS guard from currentToFiles: fetchGitDiff already bounds perFileStats at MAX_FILES (=50), and returns an empty map when the diff exceeds MAX_FILES_FOR_DETAILS upstream, so the 500-entry counter could never fire.
| } | ||
|
|
||
| let cancelled = false; | ||
| setLoading(true); |
There was a problem hiding this comment.
[Suggestion] Opening /diff eagerly computes getTurnDiff() for every real user turn. With up to 100 retained snapshots, this can fan out into many backup reads and structured diff computations before the user selects any historical turn.
Please load per-turn diffs lazily for the selected turn, or load only lightweight metadata initially and cache/limit concurrent getTurnDiff() calls.
— gpt-5.5 via Qwen Code /review
There was a problem hiding this comment.
Acknowledged — deferring to a follow-up PR. Rationale:
- The critical memory risk that motivated lazy loading is now mitigated at the source by the per-file
MAX_DIFF_SIZE_BYTEScap (see #3257766252 reply). Per-turn fan-out is bounded byMAX_SNAPSHOTS = 100and themakeSnapshotfast-path skip (unchanged backup pointers → no I/O) keeps the typical-session cost low. - Going lazy means restructuring the source-switcher tab labels (which currently rely on per-turn presence to filter empty turns) into a two-phase "list candidates → fetch on focus" flow. That's a meaningful UX redesign and worth landing in its own PR.
Will open a follow-up issue tagged perf after this merges. Marking as deferred rather than resolved so it stays on your radar.
| setLoading(true); | ||
| Promise.all([ | ||
| fetchGitDiff(cwd).catch(() => null), | ||
| fetchGitDiffHunks(cwd).catch(() => new Map<string, Hunk[]>()), |
There was a problem hiding this comment.
[Suggestion] The hook fetches full current diff hunks as soon as the dialog mounts, even while the user is only viewing the file list or may switch to a per-turn source. fetchGitDiffHunks() runs git diff HEAD and buffers/parses the full output before parser caps apply, so opening /diff can pay a large hunk-generation cost unnecessarily.
Please fetch current hunks lazily when entering detail mode for the selected current file, or add a scoped hunk API for one file and call it on demand.
— gpt-5.5 via Qwen Code /review
There was a problem hiding this comment.
Acknowledged — deferring to the same follow-up as #3257766265.
For the immediate memory concern, fetchGitDiffHunks is bounded by MAX_FILES (50) and MAX_DIFF_SIZE_BYTES (1 MB per file) inside parseGitDiff, so the worst-case eager fetch is ~50 MB stdout buffer (capped by runGit's maxBuffer). Adding a single-file scoped hunk API would be cleaner but is a non-trivial new surface on the core utility — better to bundle it with the lazy-load refactor.
…p, sanitization, Ctrl+C routing Five review-driven fixes; details inline on the PR. Core (getTurnDiff): - Treat unreadable backup files as "unavailable" (return null for the row) instead of coercing to '' and fabricating phantom hunks. Same guard for both before and after endpoints. - Cap structuredPatch input at MAX_DIFF_SIZE_BYTES so a single multi-MB file in history can no longer balloon TUI memory when /diff opens. Oversized rows still surface in the file list with best-effort line stats and a new `oversized` flag. CLI (DiffDialog): - Distinguish over-large dirty trees (filesCount > 0 but empty perFileStats) from a clean tree; the empty state now reports the capped file count and totals instead of claiming "Working tree is clean." - Render the `oversized` flag with an explicit "(oversized — diff omitted)" tag in the file list and a corresponding detail-view note. Sanitization (#4): - Move sanitizeFilenameForDisplay from diffCommand.ts into the shared textUtils module, apply it to every path rendered in DiffDialog (file rows, detail header, empty messages, DiffRenderer filename prop, generated unified-diff envelope), and keep raw paths for map lookups via a separate UnifiedFile.displayPath field. Ctrl+C routing (#7): - Register isDiffDialogOpen / closeDiffDialog with useDialogClose so Ctrl+C dismisses the dialog through the centralized handleExit path, matching how the background-tasks dialog is wired. Drop the dialog's internal Ctrl+C handler to avoid double-fire that would close the dialog AND escalate to the exit prompt. Tests: 2 new core regression tests (unreadable backup, oversized cap) plus the existing 35 still pass. CLI tests for diff/slashCommand/ AppContainer paths unchanged at 148/148.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review 总结
整体设计思路清晰：核心逻辑下沉到 FileHistoryService.getTurnDiff()，UI 通过两个独立 hook 消费数据，职责划分合理。diffCommand 的交互模式从「执行 git + 渲染」改为「打开对话框」的重构方向也是正确的。
但在实现细节上存在 2 个 P0 缺陷会导致功能不完整（新建/删除文件在 per-turn diff 中被静默丢弃），需要在合并前修复。
必须修复（P0/P1）
| # | 文件 | 问题 |
|---|---|---|
| 1 | fileHistoryService.ts |
Fast-path 当 before 和 after 都不含某文件时误判为「无变化」，新建文件被丢弃 |
| 2 | fileHistoryService.ts |
afterBackup 缺失时 fallback 用 beforeContent 代表 after 状态，文件删除事件被忽略 |
| 3 | DiffDialog.tsx |
isNewFile === isUntracked 语义错误，混淆 staged new file 与 untracked file |
| 4 | fileHistoryService.test.ts |
缺少新建文件、删除文件、failed backup 的测试用例 |
建议修复（P2/P3）
| # | 文件 | 问题 |
|---|---|---|
| 5 | useDiffData.ts |
"Current" tab 只在 mount 时 fetch 一次，数据陈旧；若有意为之请加注释说明 |
| 6 | useTurnDiffs.ts |
竞态保护不完整，已取消的 effect 仍有在途 I/O |
| 7 | DiffDialog.tsx |
hunksToUnifiedDiff 对空 hunks 返回仅含 header 的非空字符串，绕过调用方的空字符串保护 |
| 8 | DiffDialog.tsx |
useEffect 做 state 修正会多触发一帧渲染 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Dead code from removed interactive diff path
The old interactive /diff path was removed from diffCommand.ts, but HistoryItemDiffStats type, MessageType.DIFF_STATS enum value, DiffStatsDisplay component + test, and diff_stats case handlers in HistoryItemDisplay.tsx and historyUtils.ts remain as dead code — nothing in the codebase creates diff_stats items anymore. These should be cleaned up to avoid maintenance burden and confusion.
Three new files (~715 lines) have zero tests
DiffDialog.tsx (558 lines), useTurnDiffs.ts (97 lines), and useDiffData.ts (60 lines) are brand new with no corresponding test files. The dialog contains 10+ keyboard navigation branches, 4 empty-state branches, tab switching, view mode toggling, and data transformation pipelines — all untested.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| promptPreview: previewOfUserItem(item), | ||
| diff, | ||
| } satisfies TurnDiffEntry; | ||
| } catch { |
There was a problem hiding this comment.
[Suggestion] useTurnDiffs eagerly loads ALL turn diffs at once via Promise.all(userTurns.map(...)) on dialog open. For sessions with 50+ turns, this blocks the UI with "Loading diff..." until every getTurnDiff completes. Consider incremental loading — at least render results as they arrive instead of waiting for all.
Additionally, the useEffect dependency array includes history, causing a full refetch on any history array reference change (e.g., during streaming).
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Deferring to the same lazy-load follow-up as #3257766265. For the immediate concerns:
historyref stability: while the dialog is open the composer is muted, soHistoryManager's useState reference is stable — the effect doesn't actually refire spuriously in normal flow.- 'Loading diff...' blocking the entire list: in b0428c5 the per-file cap (
MAX_TURN_DIFF_FILES = 500) plus binary/oversized short-circuits bring typical cold-start under a second. Incremental rendering would still be a UX win for very-long sessions and will land with the lazy refactor.
| ); | ||
| } | ||
|
|
||
| function FileDetail({ |
There was a problem hiding this comment.
[Suggestion] turnFileToUnified hardcodes truncated: false. Unlike perFileToUnified which checks MAX_LINES_PER_FILE (400), turn diffs have no line-count guard. A turn modifying a 10,000-line generated file will render the full diff with no truncation indicator.
Add MAX_LINES_PER_FILE check and expose a truncated flag on TurnFileDiff.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Partially addressed. For oversized files the new oversized: true flag (landed in 9a63bb6) skips hunk generation entirely — so a 10,000-line turn-diff would either be oversized (>1 MB) or render fully. Adding a separate line-count truncated flag below the byte cap would be a clearer UX signal; I'll bundle it with the lazy-load follow-up rather than carry yet another flag through the pipeline in this PR. Tracking in the deferred-perf thread.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Test submission — single comment to identify line resolution issue.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Critical issues to fix before merge:
- [typecheck]
TurnDiffandTurnFileDiffare exported fromfileHistoryService.tsbut not re-exported from the core package'sindex.ts. Consumers get TS2305. - [review]
DiffDialog.tsx,useTurnDiffs.ts, anduseDiffData.tshave zero test coverage — every other dialog and hook has tests. - [review]
fileHistoryService.test.tsis missing boundary cases:isDeletedflag,afterBackup.failedskip, same-backup fast-path optimization. - [review] Dead code:
HistoryItemDiffStatstype,DiffStatsDisplaycomponent,DIFF_STATSunion member — the interactive path returns{ type: 'dialog' }instead. - [Suggestion]
diffCommand.tsnon-interactive error path exposes rawerror.messagein user-visible output. Replace with a generic label.
Detection: tsc (3 actionable type errors), eslint clean, 7 review agents (Correctness/Security/CodeQuality/Performance/TestCoverage/Oncall/Maintainer). Core tests 35/35, CLI diffCommand 25/25.
| ); | ||
| } | ||
|
|
||
| if (uiState.isDiffDialogOpen) { |
There was a problem hiding this comment.
[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
There was a problem hiding this comment.
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.
…, semantics Addresses 14 of 19 outstanding review comments. Per-thread detail will be replied on the PR. Correctness (P0): - Restrict getTurnDiff candidates to keys(target.trackedFileBackups). Files first tracked in turn N+1 no longer get phantom-attributed to turn N. Drop the now-redundant union with state.trackedFiles for the latest-turn case (makeSnapshot guarantees state.trackedFiles ⊆ keys(latest.trackedFileBackups)). - Add `beforeBackup !== undefined` guard to the fast-path skip so a future broadening of the candidate set can't silently collapse a newly created file as "unchanged". - Add binary detection via NUL-byte sniff (`looksBinary`, mirrors git's heuristic). New `TurnFileDiff.isBinary` flag short-circuits hunk generation; the dialog renders the existing italic "binary" marker instead of feeding raw bytes to DiffRenderer. - Cap per-turn concurrent file reads at MAX_TURN_DIFF_FILES=500 so a 500-file turn won't issue 1000+ simultaneous open()s and hit the process fd ceiling. UX / stability: - Stabilize the dialog's keypress handler with `useCallback(()=>..,[])` reading state via refs, eliminating subscribe/unsubscribe churn on every render. - Disentangle `isNewFile` (snapshot-derived, "added in this turn") from `isUntracked` (git "never tracked") in perFileToUnified so untracked files no longer get mislabeled as "(new)" — they could not be recovered by /rewind, and the wrong tag implied otherwise. - Reorder FileRow tag priority around the disentangled flags; remove duplicate "(binary)" tag (the stats column already shows it italic). - Drop the early-exit `useEffect` clamps for sourceIndex / fileIndex in favor of inline `Math.min` derivations; effect-based clamping caused an extra render frame that could look like a flicker in Ink. - Inner `cancelled` checks in useTurnDiffs reduce wasted disk I/O when the dialog is closed mid-load. - Guard hunksToUnifiedDiff against empty hunk arrays (would otherwise hand DiffRenderer a header-only string). - Surface "…and N more (showing first M)" indicator for the Current source when fetchGitDiff capped perFileStats at MAX_FILES. - useDiffData JSDoc clarifies the snapshot-at-open semantics; catch branches now console.debug the underlying error instead of swallowing silently. Tests: - 3 new core regression tests: deleted-during-turn detection, binary detection, and the cross-turn attribution boundary. fileHistoryService tests now at 40/40. Pending review comments (deferred): the lazy-load suggestions remain intentionally deferred per the earlier reply chain; the MAX_DIFF_SIZE cap landed in the prior round mitigated the underlying memory risk.
| // the same constant the git path uses (MAX_FILES_FOR_DETAILS = 500 | ||
| // files total), with two reads each → 1000 open()s worst case, still | ||
| // comfortably below the typical 4096 fd ceiling. | ||
| const cappedPaths = candidatePaths.slice(0, MAX_TURN_DIFF_FILES); |
There was a problem hiding this comment.
[Critical] candidatePaths.slice(0, MAX_TURN_DIFF_FILES) 静默截断且顺序不确定
- 截断到 500 个文件无
debugLogger.warn，无 UI 提示（hiddenFileCount模式仅用于 "Current" 源）。 Object.keys()迭代顺序在 JS 引擎间不保证一致，导致截断结果不可复现。
| const cappedPaths = candidatePaths.slice(0, MAX_TURN_DIFF_FILES); | |
| const sorted = candidatePaths.slice().sort((a, b) => a.localeCompare(b)); | |
| if (sorted.length > MAX_TURN_DIFF_FILES) { | |
| debugLogger.warn( | |
| `getTurnDiff truncating ${sorted.length - MAX_TURN_DIFF_FILES} files ` + | |
| `for prompt ${promptId} (cap: ${MAX_TURN_DIFF_FILES})`, | |
| ); | |
| } | |
| const cappedPaths = sorted.slice(0, MAX_TURN_DIFF_FILES); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in 5cf48b0.
candidatePaths is now sorted via localeCompare before the
MAX_TURN_DIFF_FILES slice — even though Object.keys() is spec-defined
as insertion order, sorting makes the kept-vs-dropped split reproducible
regardless of how the snapshot was populated (live session vs. resumed
from disk). When the cap kicks in we now emit a
debugLogger.warn('FileHistory: getTurnDiff truncating <N> files for prompt <id> (cap: 500)') so the dropped count is at least traceable in
--debug runs.
Surface-level UI signal stays deferred to the same lazy-load follow-up
as #3257997114 — hiddenFileCount would still be the natural home, but
plumbing it across the per-turn source pipeline (which only ever sees
the post-cap file list) is out of scope for this PR.
| hunks: [], | ||
| isNewFile: !beforeExists && afterExists, | ||
| isDeleted: beforeExists && !afterExists, | ||
| linesAdded: 0, |
There was a problem hiding this comment.
[Critical] 二进制检测返回 linesAdded: 0, linesRemoved: 0 导致统计不一致
二进制提前返回硬编码零值统计。仅修改二进制文件的回合在对话框头部显示 "+0 -0"，与 "Current" git 源的真实统计不一致。
| linesAdded: 0, | |
| // Compute best-effort line counts even for binary files | |
| const beforeLines = beforeExists ? countLines(beforeContent) : 0; | |
| const afterLines = afterExists ? countLines(afterContent) : 0; | |
| return { | |
| filePath, | |
| hunks: [], | |
| isNewFile: !beforeExists && afterExists, | |
| isDeleted: beforeExists && !afterExists, | |
| linesAdded: Math.max(0, afterLines - beforeLines), | |
| linesRemoved: Math.max(0, beforeLines - afterLines), | |
| oversized: false, | |
| isBinary: true, | |
| }; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Not adopting — the proposed change would actually introduce the
inconsistency. The "Current" git source also reports +0 / -0 for
binary files (see parseGitNumstat in packages/core/src/utils/gitDiff.ts:367-368):
const isBinary = addStr === '-' || remStr === '-';
const fileAdded = isBinary ? 0 : parseInt(addStr, 10) || 0;
const fileRemoved = isBinary ? 0 : parseInt(remStr, 10) || 0;git diff --numstat itself emits -\t-\tpath for binary files, and we
already collapse that to 0/0 in stats while flagging isBinary: true.
That matches the well-established convention you see in git diff --stat
("Bin <N> -> <M> bytes" rather than line counts) — counting lines of a
binary blob is meaningless and would produce mismatched semantics
between the two sources.
Per-turn +0 / -0 for binary files is therefore consistent with what
Current shows for the same file, which I think is the more important
invariant. Leaving as-is.
| // A non-null backup name that fails to read means we cannot produce a | ||
| // trustworthy "before" content — fabricating an empty string would | ||
| // present every line as a fresh addition. Skip the row instead. | ||
| if (beforeRead === 'unreadable') return null; |
There was a problem hiding this comment.
[Critical] 不可读端点静默丢弃，零诊断信号
当 readEndpointContent 返回 'unreadable'（备份文件损坏/权限变更/竞态删除）时，computeTurnFileDiff 返回 null 且无 debugLogger.warn。该文件已有的 debugLogger 导入，添加日志成本为零。
| if (beforeRead === 'unreadable') return null; | |
| if (beforeRead === 'unreadable') { | |
| debugLogger.warn( | |
| `Skipping turn diff for ${trackingPath}: before backup is unreadable`, | |
| ); | |
| return null; | |
| } |
对 afterRead === 'unreadable'（L798）同理。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in 5cf48b0. Both unreadable branches now emit a debugLogger.warn
before dropping the row:
FileHistory: skipping turn diff for <path>: before backup unreadable
FileHistory: skipping turn diff for <path>: after <worktree|backup> unreadable
We still drop the row rather than fabricate an empty-string endpoint
(that's the bug the prior round fixed), but now the drop leaves a
trace in --debug so backup corruption, permission flips, and
EISDIR races are diagnosable instead of invisible.
Added warn: vi.fn() to the mocked createDebugLogger in
fileHistoryService.test.ts so the new call paths don't crash the
existing regressions.
| Promise.all([ | ||
| fetchGitDiff(cwd).catch((err) => { | ||
| // eslint-disable-next-line no-console | ||
| console.debug('[DiffDialog] fetchGitDiff failed:', err); |
There was a problem hiding this comment.
[Suggestion] console.debug 应使用 createDebugLogger 匹配项目约定
项目有 177 处 createDebugLogger 调用；此处用裸 console.debug 并压制 ESLint 规则。createDebugLogger 提供一致的过滤和日志行为。
| console.debug('[DiffDialog] fetchGitDiff failed:', err); | |
| const log = createDebugLogger('useDiffData'); | |
| // ... | |
| fetchGitDiff(cwd).catch((err) => { | |
| log.debug('fetchGitDiff failed:', err); | |
| return null; | |
| }), |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in 5cf48b0. useDiffData now imports createDebugLogger from
@qwen-code/qwen-code-core and uses a DiffDialog-tagged logger
instead of the bare console.debug + eslint-disable pair:
const debugLogger = createDebugLogger('DiffDialog');
// …
fetchGitDiff(cwd).catch((err) => {
debugLogger.debug(`fetchGitDiff failed: ${err}`);
return null;
}),Aligns with the other 177 usages in the codebase and inherits the
project's debug-namespace filtering.
| } | ||
| if (name === 'return') { | ||
| const sel = selectedFileRef.current; | ||
| if (sel && !sel.isBinary && !sel.oversized) { |
There was a problem hiding this comment.
[Suggestion] 对二进制/超尺寸文件按 Enter 无视觉反馈
if (sel && !sel.isBinary && !sel.oversized) 在条件不满足时静默消费按键。虽然文件行标记了原因，但按键本身无反馈可使用户怀疑对话框冻结。
建议在守卫中添加提示：例如在对话框底部短暂显示 "Binary files cannot be previewed" 或 "File diff omitted (oversized)"。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in 5cf48b0. The dialog now surfaces a transient warning-colored
hint in the footer when Enter lands on a non-enterable row, instead of
silently consuming the keypress:
- Binary file →
Binary file — no diff to view. - Oversized file →
Oversized file — diff omitted. Use \git diff` to inspect.` - No hunks (e.g. untracked / capped) →
No diff content available for this file.
The hint replaces the regular nav line (←/→ source · ↑/↓ file · …) and
clears the moment the user presses any navigation key, so it doesn't
linger past their response. Color comes from theme.status.warning to
match the existing nudge surfaces.
Implementation: a single keyHint state, surfaced via setKeyHintRef
so the stable useCallback([], ...) keypress handler can write it
without forcing the callback to re-create.
… binary tail scan Six review-driven correctness fixes; details inline on the PR. Core: - `readEndpointContent` now distinguishes ENOENT (genuine deletion) from other read failures (EACCES/EISDIR/EBUSY/decoding) on the live worktree branch. Previously every failure collapsed to `exists:false` and produced a phantom delete hunk for files whose perms changed mid-session. - `computeTurnFileDiff` is wrapped in per-file try/catch so a single `structuredPatch` crash or transient read error can no longer poison the whole turn's `Promise.all` and silently erase every row. - `looksBinary` now scans both the head AND the tail of the string. The head-only scan could be defeated by an 8KB+ text prefix in front of a binary payload; the oversized cap (1 MB) bounds the work either way. - `getTurnDiff` calls the existing `findSnapshotIndex` helper instead of inlining a duplicate reverse-scan loop, so a future change to `findSnapshot`'s tie-break rules can't silently desync /rewind and /diff. UI: - Add `hasHunks` to `UnifiedFile` and gate Enter on it. Untracked files don't appear in `git diff HEAD` output, and capped/oversized turn entries have empty hunks — pressing Enter on those previously landed the user on a dead-end "No hunks available" screen. - Drop the misleading `total > MAX_LINES_PER_FILE` heuristic from `perFileToUnified`'s `truncated` flag. `s.truncated` (from `parseGitNumstat`) is the only authoritative source — the OR was conflating "untracked file too big to count" with "tracked file with many accurately-counted lines", incorrectly flagging the latter. Tests: - 1 new core regression test: live-worktree EISDIR failure must not be reported as a deletion. fileHistoryService tests now at 41/41.
|
Hi @wenshao @doudouOUC — re-requesting review. The latest commit
Total across the PR: 45 / 50 review threads resolved. Open threads are intentionally deferred (lazy-load perf follow-up + global ErrorBoundary) with replies explaining the rationale. Tests: core 41/41, CLI 148/148, typecheck + lint clean on both packages. |
# Conflicts: # packages/cli/src/ui/AppContainer.tsx # packages/cli/src/ui/hooks/useDialogClose.ts
| return true; | ||
| } | ||
|
|
||
| if (options.isDiffDialogOpen && options.closeDiffDialog) { |
There was a problem hiding this comment.
[Suggestion] Priority inversion with background-tasks dialog in Ctrl+C handler.
DialogManager.tsx renders DiffDialog before BackgroundTasksDialog (so when both flags are true, the user sees the diff dialog). But this hook checks isBackgroundTasksDialogOpen (line 152) before isDiffDialogOpen (this line). When both are open, Ctrl+C dismisses the background-tasks dialog while the user sees the diff dialog on screen — the user must press Ctrl+C a second time.
Consider swapping the order so the diff dialog check appears before the background-tasks check, matching the rendering priority in DialogManager.tsx.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
There was a problem hiding this comment.
Fixed in 5cf48b0. useDialogClose now checks isDiffDialogOpen before
isBackgroundTasksDialogOpen, matching DialogManager's render order
(DiffDialog block at L490, BackgroundTasksDialog at L507). With both
flags true, Ctrl+C now dismisses the diff dialog — the thing the user
actually sees — instead of silently closing the background-tasks
dialog hidden behind it. Single-keypress behavior restored.
Added a short comment in the hook noting the invariant so the next
person who reorders these doesn't reintroduce the inversion.
|
|
||
| function previewOfUserItem(item: HistoryItem): string { | ||
| if (item.type !== 'user' || !item.text) return ''; | ||
| const oneLine = item.text.replace(/\s+/g, ' ').trim(); |
There was a problem hiding this comment.
[Suggestion] previewOfUserItem reads raw item.text without ANSI escape sanitization — bypasses the defense applied by the chat history display path.
The primary chat rendering applies escapeAnsiCtrlCodes to history items (HistoryItemDisplay.tsx:111). This function reads the same HistoryItem.text but skips that sanitization. When the prompt preview is rendered in the DiffDialog header, any ANSI/OSC escape sequences in the user's prompt text (e.g., from pasted terminal output with color codes, or crafted prompts containing OSC 8 hyperlinks) reach the terminal unsanitized.
| const oneLine = item.text.replace(/\s+/g, ' ').trim(); | |
| function previewOfUserItem(item: HistoryItem): string { | |
| if (item.type !== 'user' || !item.text) return ''; | |
| // Sanitize ANSI/OSC escapes — chat history uses escapeAnsiCtrlCodes | |
| // for the same text; the diff dialog header bypasses that path. | |
| const safe = item.text.replace(/\x1b\[[0-9;]*[a-zA-Z]/g, '').replace(/\x1b\].*?[\x07\x1b\\]/g, ''); | |
| const oneLine = safe.replace(/\s+/g, ' ').trim(); | |
| if (oneLine.length <= PREVIEW_MAX) return oneLine; | |
| return `${oneLine.slice(0, PREVIEW_MAX - 1)}…`; | |
| } |
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
There was a problem hiding this comment.
Fixed in 5cf48b0. previewOfUserItem now routes item.text through the
shared escapeAnsiCtrlCodes from packages/cli/src/ui/utils/textUtils.ts
before whitespace-collapsing and length-truncating:
const safe = escapeAnsiCtrlCodes(item.text);
const oneLine = safe.replace(/\s+/g, ' ').trim();Reusing the shared helper instead of inlining a regex keeps the
sanitization rules consistent with the chat surface
(HistoryItemDisplay.tsx:111) — both surfaces are now defended by
the same code path, so future updates to the escape-list (extending
OSC handling, adding C1 control bytes, etc.) propagate automatically.
A pasted ANSI/OSC payload now reaches the source-tab label as
JSON-escaped text (\x1b[...m) instead of as live terminal escapes.
| before: FileHistorySnapshot, | ||
| after: FileHistorySnapshot | undefined, | ||
| ): Promise<TurnFileDiff | null> { | ||
| const filePath = this.maybeExpandFilePath(trackingPath); |
There was a problem hiding this comment.
[Suggestion] TurnFileDiff.filePath uses maybeExpandFilePath(trackingPath), producing absolute paths — inconsistent with the "Current" tab which uses repo-relative paths from fetchGitDiff.
In the DiffDialog, the "Current" source renders paths like packages/cli/src/ui/components/DiffDialog.tsx, while per-turn sources render /Users/wenshao/git/.../packages/cli/src/ui/components/DiffDialog.tsx. On narrow terminals, absolute paths are more aggressively truncated by truncatePathStart, losing project context (e.g., the packages/cli scope).
Consider returning trackingPath as filePath instead — it is already repo-relative and matches the convention used by the Current source.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
There was a problem hiding this comment.
Fixed in 5cf48b0. TurnFileDiff.filePath is now trackingPath directly
(which maybeShortenFilePath already made repo-relative for in-cwd
files), matching the convention fetchGitDiff uses for the "Current"
source. The absolute form lives only in a local absoluteFilePath
variable inside computeTurnFileDiffUnsafe and is used solely for the
live-worktree readEndpointContent call.
Verified that the existing regressions still pass after I updated the
test expectations from f.filePath === file (absolute) to
f.filePath === basename(file) (relative).
Side effect: the patch headers we pass into structuredPatch are now
also repo-relative, but those headers are internal to the diff
package's hunk struct and we discard them in the renderer, so no
user-facing change there.
Narrow-terminal context loss (/Users/.../packages/cli/... chopped to
...e/DiffDialog.tsx) is gone — both sources now render the same
packages/cli/... prefix.
|
|
|
|
- Deterministic candidate cap in getTurnDiff: sort trackedFileBackups
keys before slicing at MAX_TURN_DIFF_FILES; emit debugLogger.warn
when truncating so the dropped count is traceable.
- Log unreadable before/after endpoints in computeTurnFileDiffUnsafe
instead of dropping rows silently — backup corruption, permission
flips and EISDIR now leave a trace.
- Return trackingPath as TurnFileDiff.filePath (already repo-relative
via maybeShortenFilePath) so per-turn rows match the Current source
on narrow terminals. The internal absolute path is kept only for
live-worktree I/O.
- useDiffData: replace bare console.debug with createDebugLogger
('DiffDialog') to match project convention.
- DiffDialog: show a transient warning-coloured hint in the footer
when Enter lands on a binary / oversized / no-hunks row (cleared on
the next navigation key) so the keypress isn't silently consumed.
- useDialogClose: swap diff-dialog and background-tasks branches to
match DialogManager render order — Ctrl+C now dismisses whichever
dialog the user actually sees when both flags are open.
- useTurnDiffs: sanitize previewOfUserItem via escapeAnsiCtrlCodes so
prompt previews on the source tabs can't reach the terminal raw
(matching the chat-history defense).
- Tests: expect repo-relative filePath in getTurnDiff regression
cases; add `warn` to the mocked debugLogger.
Refs PR #4277 review comments 3259062434, 3259062465, 3264541365,
3259062480, 3259062498, 3264541346, 3264541351.
| Promise.all( | ||
| userTurns.map(async (item, idx) => { | ||
| // Early-exit so a quick close → reopen doesn't keep paying for | ||
| // disk reads from the previous effect. The outer cancellation |
There was a problem hiding this comment.
[Critical] Cross-turn I/O fan-out: Promise.all(userTurns.map(...)) fires getTurnDiff for all user turns simultaneously. Each getTurnDiff can open up to MAX_TURN_DIFF_FILES × 2 = 1000 file descriptors. With 50 turns, this is 50,000 concurrent readFile calls — exceeding ulimit -n on most systems (256 on macOS) and causing EMFILE errors that silently cascade through unrelated file operations.
The per-call cap (500 files) is safe for a single turn, but the N-way multiplication across turns is unbounded. The cancelled guard only suppresses setState — all I/O completes regardless.
Consider batching turns in groups of 3-5, or adding a concurrency semaphore:
const CONCURRENCY = 4;
for (let i = 0; i < userTurns.length; i += CONCURRENCY) {
if (cancelled) break;
const batch = userTurns.slice(i, i + CONCURRENCY);
// ... process batch
}— qwen-latest-series-invite-beta-v28 via Qwen Code /review
There was a problem hiding this comment.
Fixed in 1628101. useTurnDiffs now batches calls at TURN_CONCURRENCY = 4
instead of one unbounded Promise.all over every user turn:
for (let i = 0; i < userTurns.length; i += TURN_CONCURRENCY) {
if (cancelled) return out;
const slice = userTurns.slice(i, i + TURN_CONCURRENCY);
const batch = await Promise.all(slice.map((item, j) => loadOne(item, i + j)));
// …
}Worst-case fd usage drops from O(turns × MAX_TURN_DIFF_FILES × 2) to
O(TURN_CONCURRENCY × MAX_TURN_DIFF_FILES × 2) ≈ 4000 — well under the
default macOS 256-and-friends ulimit problem you flagged, and within
common 4096 ceilings. cancelled is now also checked between batches,
so close→reopen really does stop the disk reads rather than just
suppressing setState.
Picked 4 because turns batch through quickly enough that loading
doesn't feel slower in practice (each getTurnDiff is fast-path-heavy
when most files share backups across turns), while still cutting the
fan-out by an order of magnitude on realistic 50-turn sessions.
| ): Promise<EndpointReadOk | 'unreadable'> { | ||
| if (worktreePath !== undefined) { | ||
| try { | ||
| const text = await readFile(worktreePath, 'utf-8'); |
There was a problem hiding this comment.
[Critical] readFile(worktreePath, 'utf-8') reads the entire file into memory before the MAX_DIFF_SIZE_BYTES check at line ~906. The oversized cap only prevents hunk expansion, not the initial allocation. A user who asks the model to write_file a 2 GB blob then opens /diff would allocate 2 GB in the Node.js heap before the cap fires. With multiple such files, the process is OOM-killed.
Consider stat()-ing the path first and returning an oversized sentinel without reading:
const stats = await stat(worktreePath).catch(() => null);
if (stats && stats.size > MAX_DIFF_SIZE_BYTES) {
return { oversized: true };
}— qwen-latest-series-invite-beta-v28 via Qwen Code /review
There was a problem hiding this comment.
Fixed in 1628101. readEndpointContent now stats both the live
worktree and the backup path before readFile, and returns a new
{ kind: 'oversized', exists } sentinel when stat.size > MAX_DIFF_SIZE_BYTES:
if (size > MAX_DIFF_SIZE_BYTES) {
return { kind: 'oversized', exists: true };
}computeTurnFileDiffUnsafe short-circuits on either endpoint being
oversized and returns the standard oversized: true TurnFileDiff
shape without ever calling readFile — so a 2 GB write_file blob
no longer lands in the Node heap just for the downstream
Buffer.byteLength check to reject it. The post-read
Buffer.byteLength > MAX_DIFF_SIZE_BYTES cap stays as defense-in-depth
for the rare case where stat.size < cap but utf-8 decoding inflates
the string past it (invalid bytes → U+FFFD).
I also extended the cap to backup files for the same reason — a
backup of a giant file is just as fatal to allocate as the live one,
and the previous code would have OOM'd on backup reads too.
Test update: the oversized regression now expects
linesAdded === 0 and linesRemoved === 0 because we no longer hold
the bytes needed to count newlines. The row's purpose is to signal
the omission, not to estimate; users get the badge and the hint
"Use `git diff` to inspect" to follow up.
| @@ -390,3 +390,50 @@ export function sanitizeSensitiveText( | |||
|
|
|||
There was a problem hiding this comment.
[Suggestion] sanitizeFilenameForDisplay is a security-relevant pure function that strips ANSI sequences and control characters from git-supplied filenames to prevent terminal injection. It has no unit tests. A regression in the control-character regex or the escape mapping could allow raw terminal escape sequences to reach the TUI via crafted filenames with no test to catch it.
Suggested test cases: (1) C0 controls (\n, \r, \x00) produce \uXXXX escapes, (2) multi-byte ANSI sequences are stripped, (3) clean filenames pass through unchanged.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
There was a problem hiding this comment.
Fixed in 1628101. Added a focused describe('sanitizeFilenameForDisplay')
block to packages/cli/src/ui/utils/textUtils.test.ts covering:
- Clean filenames pass through unchanged — repo paths, non-ASCII
(文件.txt), empty string. - C0 control bytes that slip past
escapeAnsiCtrlCodes—
\n,\r,\t,\b,\f, NUL, BEL — produce\\n/
\\r/\\t/\\b/\\f/\\u0000/\\u0007escapes. - DEL (0x7F) and C1 controls (0x80–0x9F) — produce
\\u00XXescapes. - Multi-byte ANSI CSI sequences —
\x1b[31mred\x1b[0msurvives
only asred, no ESC bytes remain. - Mixed crafted path (
evil\x1b[2K\npath\x00.txt) — exercises
both regex passes together; output preserves visible substrings and
contains no raw C0 / DEL bytes (verified by char-code scan).
Total: 29 tests pass, including the 5 new ones.
| let cancelled = false; | ||
| setLoading(true); | ||
|
|
||
| const userTurns = history.filter(isRealUserTurn) as HistoryItem[]; |
There was a problem hiding this comment.
[Suggestion] Double unsafe as cast bypasses TypeScript type narrowing:
const userTurns = history.filter(isRealUserTurn) as HistoryItem[];
// ...
const promptId = (item as HistoryItemUser).promptId;If isRealUserTurn were changed to allow non-user items, or if HistoryItemUser lost promptId, TypeScript would not catch the regression.
Make isRealUserTurn a type predicate to eliminate both casts:
export function isRealUserTurn(item: HistoryItem): item is HistoryItemUser {— qwen-latest-series-invite-beta-v28 via Qwen Code /review
There was a problem hiding this comment.
Fixed in 1628101. isRealUserTurn is now a TypeScript predicate:
export function isRealUserTurn(
item: HistoryItem,
): item is HistoryItem & HistoryItemUser {
// …
}The intersection form is needed because HistoryItem = HistoryItemWithoutId & { id: number }
while HistoryItemUser doesn't carry id — narrowing to plain
HistoryItemUser would actually widen the input. The & keeps the
id part and adds the user-specific shape, so the predicate is
strictly narrowing.
Both as HistoryItem[] and (item as HistoryItemUser).promptId are
gone from useTurnDiffs:
const userTurns = history.filter(isRealUserTurn); // UserTurn[] naturally
// …
const { promptId } = item; // already narrowedA regression that loosens either isRealUserTurn or removes
promptId from HistoryItemUser will now fail tsc instead of
silently bypassing the narrowing.
| debugLogger.debug(`fetchGitDiffHunks failed: ${err}`); | ||
| return new Map<string, Hunk[]>(); | ||
| }), | ||
| ]).then(([statsRes, hunksRes]) => { |
There was a problem hiding this comment.
[Suggestion] The Promise.all([...]).then(...) chain has no .catch(). While inner promises are individually .catch()-guarded, if the .then() callback throws (e.g., React state update error during unmount), the resulting rejection is unhandled. In Node.js ≥ 22, unhandled rejections terminate the process.
Add a trailing .catch():
}).then(([statsRes, hunksRes]) => {
if (cancelled) return;
setResult(statsRes);
setHunks(hunksRes);
setLoading(false);
}).catch((err) => {
debugLogger.debug(`diff data pipeline failed: ${err}`);
if (!cancelled) setLoading(false);
});The same pattern applies to useTurnDiffs.ts line 85.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
There was a problem hiding this comment.
Fixed in 1628101. Both useDiffData and useTurnDiffs now terminate
their Promise chains with a trailing .catch():
loadAll()
.then((entries) => {
if (cancelled) return;
entries.reverse();
setTurns(entries);
setLoading(false);
})
.catch((err) => {
debugLogger.debug(`useTurnDiffs pipeline failed: ${err}`);
if (!cancelled) setLoading(false);
});Same pattern on useDiffData. The inner .catch() blocks already
handle expected I/O failures; the trailing .catch() is defense in
depth against a setState-during-unmount throw or a future refactor
that introduces a synchronous error inside .then. Without it,
Node ≥ 22 would terminate the process via the default
unhandled-rejection handler — exactly the failure mode you flagged.
Both catch branches log via createDebugLogger('DiffDialog') and
unstick loading so the dialog falls through to its existing
empty-state rendering instead of spinning forever.
| return true; | ||
| } | ||
|
|
||
| // Order must match `DialogManager`'s render priority — the diff |
There was a problem hiding this comment.
[Suggestion] The comment says "Order must match DialogManager's render priority" but the actual ordering differs: in DialogManager, DiffDialog renders near position 35 (after most dialogs), but here it is checked at position ~12 (after WelcomeBackDialog, before BackgroundTasksDialog). Multiple dialogs (Help, Memory, Arena, RewindSelector) render above DiffDialog in DialogManager but are checked before it here.
The "must match" contract is misleading and not enforced. If a new dialog is added between rewind and diff in DialogManager without a matching update here, Ctrl+C would close the wrong dialog. Consider extracting the close priority order into a shared constant.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
There was a problem hiding this comment.
Fixed (comment scope) in 1628101. You're right — the previous "Order must
match DialogManager's render priority" comment overclaimed; the hook
deliberately doesn't mirror the full DialogManager order (theme /
auth / settings / … each have their own priority elsewhere in this
hook). The actual invariant the swap was protecting is narrower:
DiffDialog must beat BackgroundTasksDialog specifically, because
DialogManager renders the diff dialog over the background-tasks
dialog when both flags are open.
New comment scopes the claim correctly:
// Scoped invariant: the diff-dialog branch MUST sit above the
// background-tasks branch because DialogManager renders the diff
// dialog over BackgroundTasksDialog when both flags are true …
// The rest of this hook's ordering is **not** a mirror of
// DialogManager and isn't intended to be …Extracting a shared priority constant is appealing but out of scope
for this PR — every dialog touches this hook on top of its own setup,
and lifting them into a central registry is a refactor that warrants
its own change. Filing as a follow-up note in my notes.
| // the same constant the git path uses (MAX_FILES_FOR_DETAILS = 500 | ||
| // files total), with two reads each → 1000 open()s worst case, still | ||
| // comfortably below the typical 4096 fd ceiling. | ||
| if (candidatePaths.length > MAX_TURN_DIFF_FILES) { |
There was a problem hiding this comment.
[Suggestion] When candidatePaths.length > MAX_TURN_DIFF_FILES, the file list is silently truncated and stats.filesChanged reflects only the capped subset. The TurnDiff type has no field indicating truncation occurred. Consequently, DiffDialog has no data to show a truncation indicator for turn sources — unlike the "Current" source which computes hiddenFileCount from stats.filesCount.
A turn editing 600 files would show "500 files +1200 -800" with no hint that 100+ additional files were dropped.
Consider adding a filesOmitted: number field to TurnDiff.stats and surfacing it in the dialog analogous to hiddenFileCount.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
There was a problem hiding this comment.
Fixed in 1628101. TurnDiff.stats now carries filesOmitted: number,
computed at the slice site in getTurnDiff:
const filesOmitted = Math.max(0, candidatePaths.length - MAX_TURN_DIFF_FILES);
if (filesOmitted > 0) {
debugLogger.warn(`getTurnDiff truncating ${filesOmitted} files …`);
}
// …
stats: { filesChanged: files.length, linesAdded, linesRemoved, filesOmitted }DiffDialog's hiddenFileCount now reads from this field for turn
sources, mirroring the existing Current-source branch:
const hiddenFileCount =
activeSource?.kind === 'current'
? Math.max(0, stats.filesCount - files.length)
: activeSource?.kind === 'turn'
? activeSource.entry.diff.stats.filesOmitted
: 0;Result: a turn editing 600 files now renders "500 files +X -Y" and
"…and 100 more (showing first 500)" beneath the list, instead of
silently hiding the gap. Same string the Current source already used,
so the UX is symmetric across sources.
| return; | ||
| } | ||
| setLoading(true); | ||
| Promise.all([ |
There was a problem hiding this comment.
[Suggestion] fetchGitDiff(cwd) and fetchGitDiffHunks(cwd) are two independent git diff HEAD subprocess invocations. If the working tree changes between them (concurrent tool edit, file watcher), perFileStats and hunks can disagree — a file may show +5 -3 in the list but have no hunks, or have hunks but zero stats.
This is a pre-existing architecture but newly exposed by the dialog which presents both datasets side-by-side for the first time. Consider either deriving both from a single git diff HEAD invocation, or surfacing a transient note when the datasets disagree.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
There was a problem hiding this comment.
Acknowledged — deferring to a follow-up. The race is real but
pre-existing arch (fetchGitDiff and fetchGitDiffHunks were two
separate git diff HEAD invocations long before this PR added a
caller that surfaces both side-by-side), so fixing it cleanly means
either:
- Merging the two into a single
git diff HEADinvocation that
returns numstat + hunks together — invasive refactor of the public
packages/core/src/utils/gitDiff.tsAPI and its non-interactive
consumers. - Diffing two child processes' git outputs against each other and
surfacing a note when they disagree — adds UI state and a
detection heuristic for a window that is microseconds wide in
practice (fetchGitDifffinishes shortly beforefetchGitDiffHunks
starts, both insidePromise.all).
For this PR the contained surface is acceptable: the dialog opens on
a frozen snapshot (snapshot-at-open semantics, already documented in
useDiffData's JSDoc), and users running concurrent tool edits while
manually opening /diff is rare enough that we haven't seen it cause
visible disagreement in testing.
Filing as a tracked follow-up under "merge git-diff invocations into
a single subprocess" — the cleanest fix is path 1, but it benefits
from being scoped on its own rather than bundled into this PR.
- readEndpointContent now stats both worktree and backup paths before
readFile and returns a `{ kind: 'oversized' }` sentinel when the
file exceeds MAX_DIFF_SIZE_BYTES. computeTurnFileDiffUnsafe handles
the sentinel without allocating, so a 2 GB write_file blob no longer
lands in the Node heap just to be rejected downstream.
- useTurnDiffs now batches `getTurnDiff` calls at TURN_CONCURRENCY = 4
instead of an unbounded Promise.all across every user turn. Prevents
EMFILE on long sessions (worst case ~4000 fds vs. unbounded N × 1000).
- Add `filesOmitted` to `TurnDiff.stats` and plumb it through the
dialog's `hiddenFileCount` so per-turn rows now also surface "…and N
more" when MAX_TURN_DIFF_FILES truncates the candidate list (matches
the Current source's existing behavior).
- Make isRealUserTurn a type predicate (`item is HistoryItem &
HistoryItemUser`) so callers in useTurnDiffs drop both `as` casts —
a future regression that loosens either side will now be caught by
tsc rather than silently bypassing the narrowing.
- Add trailing `.catch()` to the Promise.all chains in useDiffData and
useTurnDiffs so a thrown setState during unmount doesn't propagate
to Node 22+'s default unhandled-rejection terminator. Both branches
log via createDebugLogger and unstick `loading`.
- Tighten the comment above the diff/background-tasks branch in
useDialogClose: the invariant is scoped to that pair, not a
full mirror of DialogManager's render priority.
- Add focused unit tests for sanitizeFilenameForDisplay (C0 controls,
DEL + C1, multi-byte ANSI CSI, mixed crafted paths, clean
passthrough) — security-relevant function previously untested.
Refs PR #4277 review comments 3265032536, 3265032548, 3265032551,
3265032556, 3265032560, 3265032569, 3265032574.
| return { filesChanged, insertions, deletions }; | ||
| } | ||
|
|
||
| interface EndpointReadOk { |
There was a problem hiding this comment.
[Suggestion] EndpointReadOk lacks an explicit kind discriminant while EndpointReadOversized has kind: 'oversized'. Lines 930-960 rely on 'kind' in checks and 6 explicit as casts (as EndpointReadOk, as EndpointReadOversized) instead of TypeScript narrowing.
Adding kind: 'ok' to EndpointReadOk and converting 'unreadable' to { kind: 'unreadable' } would make this an idiomatic discriminated union, eliminate all manual casts, and let the compiler catch missing branches:
interface EndpointReadOk { kind: 'ok'; content: string; exists: boolean; }
interface EndpointReadOversized { kind: 'oversized'; exists: boolean; }
interface EndpointReadUnreadable { kind: 'unreadable' }
type EndpointRead = EndpointReadOk | EndpointReadUnreadable | EndpointReadOversized;Then all typeof === 'object' && 'kind' in narrowing and as casts become simple if (read.kind === 'oversized') switches.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| try { | ||
| const st = await stat(worktreePath); | ||
| size = st.size; | ||
| } catch (e: unknown) { |
There was a problem hiding this comment.
[Suggestion] The pre-read stat() then readFile() are two separate syscalls with no atomicity. If the file grows past MAX_DIFF_SIZE_BYTES between the two calls (e.g., a concurrent write_file tool call appending to the same file), the full content is loaded into the Node heap — defeating the OOM guard.
Using fs.open() + fstat(fd) + readFile({ fd }) eliminates the TOCTOU race by operating on the same inode:
const fh = await open(worktreePath, 'r');
try {
const st = await fh.stat();
if (st.size > MAX_DIFF_SIZE_BYTES) return { kind: 'oversized', exists: true };
const text = await readFile(fh, 'utf-8');
return { content: text, exists: true };
} finally {
await fh.close();
}The window is narrow but the consequence (heap spike up to full file size) is severe — especially during active agent writes.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| // the same constant the git path uses (MAX_FILES_FOR_DETAILS = 500 | ||
| // files total), with two reads each → 1000 open()s worst case, still | ||
| // comfortably below the typical 4096 fd ceiling. | ||
| const filesOmitted = Math.max( |
There was a problem hiding this comment.
[Suggestion] filesOmitted counts candidates dropped by MAX_TURN_DIFF_FILES before diff computation, while stats.filesChanged (line 829) counts only files with non-null diffs after. The two use different denominators: if a turn touches 600 files (cap 500) and 50 of the 500 processed files have no changes, the dialog shows "450 files + 100 more" but 450 + 100 = 550 ≠ 600.
Consider computing the hidden count as totalCandidates - files.length in the dialog (matching the Current source's stats.filesCount - files.length pattern) so the accounting is consistent across both source kinds.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| /** Number of candidate files dropped because the turn touched more | ||
| * than `MAX_TURN_DIFF_FILES`. Mirrors the Current source's | ||
| * `hiddenFileCount` so the dialog can show "…and N more". */ | ||
| filesOmitted: number; |
There was a problem hiding this comment.
[Suggestion] Three test coverage gaps in the new OOM/safety code:
-
filesOmittedis computed at line 797, set onstatsat line 832, consumed byDiffDialog.tsx— but no test asserts on it (not even baseline=== 0). A regression in the cap math would silently break the dialog's truncation indicator. -
The worktree stat guard in
readEndpointContent(lines 358-390) is never exercised by the existing oversized test, which uses two backup-based snapshots. To hit this path, a test needs a single snapshot where the live worktree file exceedsMAX_DIFF_SIZE_BYTES. -
The mixed-size endpoint case (small before + oversized after) in
computeTurnFileDiffUnsafeuses different type-cast logic than the both-oversized case but is untested.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| * enough that loading never feels slower in practice while bounding the | ||
| * worst case to ~4000 concurrent fds (well under typical 4096 ceilings). | ||
| */ | ||
| const TURN_CONCURRENCY = 4; |
There was a problem hiding this comment.
[Suggestion] useTurnDiffs contains substantial logic — concurrency batching (TURN_CONCURRENCY = 4), cancellation via cancelled flag, per-turn error swallowing, most-recent-first ordering, empty-diff filtering — but has no test file. A renderHook test covering at minimum: (1) empty turns are filtered, (2) output is most-recent-first, (3) getTurnDiff errors for individual turns don't block others, and (4) all results arrive when loading >4 turns would catch regressions in the batching/cancellation logic.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
3 participants
Summary
Closes #4272.
Reworks
/diffso users can drill into the changes from each conversation turn — not just the aggregate working-tree-vs-HEAD summary that the command used to print.In interactive mode
/diffnow opens a dialog with:git diff HEAD) plus one entry per past user turn (T1 · T2 · …, most recent first)+N -M, and tags for new / deleted / untracked / binaryNon-interactive and ACP paths keep the existing plain-text summary unchanged so pipes, logs, and remote transports are unaffected.
How it works
Per-turn data comes from the existing
FileHistoryServicesnapshots (one per user query, seeclient.ts:1278). New methodFileHistoryService.getTurnDiff(promptId)walks every file tracked by the target turn's snapshot and compares it against the next snapshot's backup — or the live worktree for the most recent turn — usingdiff.structuredPatch. Files the snapshotter failed to capture are skipped instead of rendered against a stale predecessor.This means turns that survived chat compression still show their diff, since snapshots live independently of the API history (capped at 100, same as
/rewind).Caveat (already true for
/rewind): only files touched viaedit/write_fileare snapshotted, so changes fromrun_shell_commandwon't appear in per-turn entries. The Current source still covers them viagit diff.Files changed
Core (2)
packages/core/src/services/fileHistoryService.ts— newTurnDiff/TurnFileDifftypes andgetTurnDiff()(~190 lines)packages/core/src/services/fileHistoryService.test.ts— 5 new unit testsCLI (8 changed + 3 new)
packages/cli/src/ui/components/DiffDialog.tsx(new) — the dialogpackages/cli/src/ui/hooks/useTurnDiffs.ts(new) — bridgesHistoryItemUser.promptIdtogetTurnDiffpackages/cli/src/ui/hooks/useDiffData.ts(new) — Current source via existingfetchGitDiff/fetchGitDiffHunkspackages/cli/src/ui/commands/diffCommand.ts— interactive path now returns{ type: 'dialog', dialog: 'diff' }packages/cli/src/ui/commands/types.ts— dialog union gains'diff'packages/cli/src/ui/hooks/slashCommandProcessor.ts— dispatcher +SlashCommandProcessorActionsaddopenDiffDialogpackages/cli/src/ui/AppContainer.tsx—isDiffDialogOpenstate, open/close handlers, included indialogsVisiblepackages/cli/src/ui/contexts/{UIStateContext,UIActionsContext}.tsx— surface the new state/actionspackages/cli/src/ui/components/DialogManager.tsx— mount<DiffDialog>packages/cli/src/ui/commands/diffCommand.test.ts— re-aligned interactive tests with the new contractTest plan
tsc --noEmitclean in bothpackages/coreandpackages/clieslintclean on changed filesvitest run—fileHistoryService.test.ts35/35,diffCommand.test.ts25/25,slashCommandProcessor.test.ts46/46,AppContainer.test.tsx+RewindSelector.test.tsx78/78/diff, page through turns, view hunks, Esc out cleanlyqwen --no-interactive /diffstill prints the plain-text summaryDesign notes
/diff("what just changed")./rewind, this dialog only reads from physical snapshots andgit diff, so the IDE-injected user Content that breaks rewind's API-history mapping doesn't affect it.trackEdit), so T1's diff is still meaningful.