feat(cli): add optional [HH:MM:SS] timestamp before each assistant turn - #5001
Conversation
_2026-06-11.193125.mp4 |
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Automated Review (qwen3-coder-plus)
Verdict: COMMENT — CI is broken due to a TypeScript type error. One high-confidence finding below.
Previously reported issues — status
-
Timestamp overwrite bug — Fixed in commit
32fe709. ThecommitItemguard now checks!(item as HistoryItemGemini).timestampbefore assigningDate.now(), preserving the turn-start time set bysetPendingHistoryItem. -
isPending guard — Resolved by design decision in commit
32fe709. The test was updated to expect timestamps during streaming (isPending=true). This is intentional — the timestamp marks when the turn started, visible immediately as the model streams.
New finding
See inline comment below.
| const afterText = newGeminiMessageBuffer.substring(splitPoint); | ||
| addItem( | ||
| commitItem( | ||
| { |
There was a problem hiding this comment.
[Suggestion] The split path constructs {type, text: beforeText} without propagating the timestamp from pendingHistoryItemRef.current. Since commitItem only stamps when !timestamp, the split chunk gets Date.now() at split time rather than the original turn-start time.
The no-split path correctly preserves the timestamp via:
...(item && 'timestamp' in item ? { timestamp: item.timestamp } : {})For long streaming responses that trigger splits, the displayed [HH:MM:SS] will reflect the split time, not when the assistant began responding.
| { | |
| commitItem( | |
| { | |
| type: pendingHistoryItemRef.current?.type as | |
| | 'gemini' | |
| | 'gemini_content', | |
| text: beforeText, | |
| ...(pendingHistoryItemRef.current && 'timestamp' in pendingHistoryItemRef.current | |
| ? { timestamp: (pendingHistoryItemRef.current as HistoryItemGemini).timestamp } | |
| : {}), | |
| }, | |
| userMessageTimestamp, | |
| ); |
— qwen3.7-max via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Re-review after fix for TS2345 type error (previously reported at 9eebd50).
Summary
The previously reported TypeScript type error in setPendingHistoryItem updater has been fixed in commit d9fc397. The fix uses explicit type assertion (HistoryItemWithoutId base with as HistoryItemGemini for timestamp propagation) instead of object spread, avoiding the incompatible return type.
Verification
- TypeScript: No type errors in the changed files.
tsc --noEmitpasses cleanly (only pre-existing TS6305 build-artifact warnings). - Tests: All 18
HistoryItemDisplaytests and all 103useGeminiStreamtests pass on CI. - CI note: The ubuntu-latest
SessionPreview.test.tsxfailures (5/6 tests) are unrelated to this PR -- that component is not touched by the diff.
Code review
commitItemwrapper correctly attachesDate.now()only togeminiitems without an existing timestamp, preserving the turn-start time fromsetPendingHistoryItem.setPendingHistoryItemupdater correctly carries forward thetimestampfield during content accumulation.- Dependency arrays in all
useCallbackhooks that usecommitItemare properly maintained. - The
gemini_contentsplit path correctly excludes timestamps to prevent duplicate display within a single turn. HistoryItemDisplayrendering logic correctly guards on bothshowTimestampssetting and non-nulltimestamp.
No high-confidence issues found.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review
|
@ZijianZhang989 heads up — this PR currently has merge conflicts with Conflicting files:
The rest merges cleanly. Thanks! 中文@ZijianZhang989 提个醒 —— 这个 PR 目前和 冲突文件:
其余文件可以自动合并。谢谢! |
84decbd
e5e0826 to
84decbd
Compare
|
Rebased onto latest E2E verified:
No functional changes to the feature itself. |
|
Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs. |
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Clean timestamp feature: showTimestamps setting (default false) renders [HH:MM:SS] before assistant responses using toLocaleTimeString('en-US', {hour12: false}). Good test coverage (4 tests: disabled, enabled, streaming, missing timestamp). useSettings integration is correct. SessionPreview tests refactored to use renderWithProviders. CI green on tests, but review-pr workflow check is failing. Downgraded from Approve to Comment: CI review-pr check failing. — claude-opus-4-6 via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Suggestion] convertToHistoryItems in resumeHistoryUtils.ts:385 creates gemini items as { type: 'gemini', text } without propagating record.timestamp. When showTimestamps is enabled, resumed sessions will show timestamps only on new messages — historical assistant turns render without timestamps, creating an inconsistent experience within the same conversation view.
Suggested fix: propagate the persisted timestamp:
items.push({
type: 'gemini',
text,
timestamp: new Date(record.timestamp).getTime(),
});— qwen3.7-max via Qwen Code /review
| @@ -2673,10 +2673,10 @@ describe('useGeminiStream', () => { | |||
| }); | |||
|
|
|||
| expect(mockAddItem).toHaveBeenCalledWith( | |||
There was a problem hiding this comment.
[Suggestion] The only test changes for commitItem are relaxed assertions (expect.objectContaining) to accommodate the new timestamp field. There are no positive tests verifying that commitItem actually attaches a timestamp to gemini items, leaves non-gemini items (gemini_content, info, tool_group) untouched, or preserves a pre-existing timestamp via the !timestamp guard.
Consider adding dedicated tests that drive the stream through a gemini text event and assert mockAddItem was called with an item containing a numeric timestamp, and a test that commits a non-gemini item and asserts no timestamp was added.
— qwen3.7-max via Qwen Code /review
| const commitItem = useCallback( | ||
| (item: HistoryItemWithoutId, userMessageTimestamp: number): number => { | ||
| if (item.type === 'gemini' && !(item as HistoryItemGemini).timestamp) { | ||
| (item as HistoryItemGemini).timestamp = Date.now(); |
There was a problem hiding this comment.
[Suggestion] commitItem mutates item in place ((item as HistoryItemGemini).timestamp = Date.now()) before passing it to addItem. Since the argument is pendingHistoryItemRef.current — a shared ref object — this is a side-effect on a React-managed object outside the state setter. While benign today (callers null the ref immediately after), it creates a hidden mutation contract that future refactors could break.
Consider cloning before augmenting:
if (item.type === 'gemini' && !(item as HistoryItemGemini).timestamp) {
return addItem({ ...item, timestamp: Date.now() }, userMessageTimestamp);
}
return addItem(item, userMessageTimestamp);— qwen3.7-max via Qwen Code /review
|
The feature itself looks good and the earlier review rounds already cleaned up the timestamp-overwrite and TS2345 issues, so no code concerns from me. One blocker before this can merge: the conflict has moved. Non-blocking — |
20a7e15 to
f8abb5d
Compare
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ On direction: Showing when each assistant turn was generated is a real user need — debugging, logging, understanding temporal flow. Claude Code's changelog has related timestamp features (transcript timestamps, Ctrl-r timestamps, memory file timestamps), so this area is well-aligned with CLI assistant tooling. No concerns here. On approach: The scope is tight and focused. Merge status: MERGEABLE, no conflicts. CI all green (9 passing, 4 skipped). 中文说明感谢贡献！ 模板完整 ✓ 方向：在每个助手 turn 前显示生成时间是一个真实的用户需求——调试、日志记录、理解时间流。Claude Code 的 CHANGELOG 里有相关的 timestamp 功能（transcript 时间戳、Ctrl-r 时间戳、memory 文件时间戳），所以这个方向跟 CLI 助手工具的发展路线一致。没有顾虑。 方案：范围紧凑聚焦。 合并状态：可合并 (MERGEABLE)，无冲突。CI 全绿（9 通过，4 跳过）。 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal: To add per-turn timestamps, I'd add an optional Comparison: The PR's approach matches this exactly. The The No correctness, security, or convention issues found. TestingUnit Tests
TUI Test (tmux interactive, fake OpenAI server)showTimestamps enabled ( Timestamp showTimestamps disabled ( No timestamp — confirms the setting works correctly in both directions. 中文说明代码审查独立方案： 要添加每个 turn 的时间戳，我会在 对比： PR 的方案完全匹配。
没有发现正确性、安全性或规范问题。 测试单元测试
TUI 测试（tmux 交互模式，fake OpenAI 服务器）showTimestamps 启用 ( 时间戳 showTimestamps 禁用 ( 没有时间戳——确认设置在两个方向都正确工作。 — Qwen Code · qwen3.7-max |
ReflectionStepping back: this PR does one thing well — adds optional The one blocker from the prior triage — the unmigrated The diff is minimal and focused: 8 files changed, 240 additions, 111 deletions. Every change serves the stated goal. The Verdict: Approve — the previous blocker is resolved, all tests pass, the TUI behavior matches the description. Ready to ship. ✅ @ZijianZhang989 中文说明总结退一步看：这个 PR 把一件事做好了——在助手 turn 前添加可选的 上次 triage 的阻塞项——未迁移的 diff 精简且聚焦：8 个文件变更，240 行增加，111 行删除。每个变更都服务于目标。 结论： 批准——之前的阻塞项已解决，所有测试通过，TUI 行为与描述一致。可以合入。✅ @ZijianZhang989 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM — feature works as described, tests pass, tmux confirms timestamp rendering. One minor ask: consider splitting the SessionPreview.test.tsx refactoring into a separate PR to keep this one focused. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing: Lint. — qwen3.7-max via Qwen Code /review
33d9a8a to
85fae9a
Compare
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Suggestion] convertToHistoryItems in resumeHistoryUtils.ts:384 pushes { type: 'gemini', text } without a timestamp field. With showTimestamps enabled, resumed sessions will silently have no timestamps on historical turns. Consider documenting this limitation in the setting's description.
[Suggestion] restoreHistoryItem in resumeHistoryUtils.ts:119 converts timestamps to Date objects (clone['timestamp'] = new Date(ts)) but HistoryItemGemini.timestamp is typed as number. Currently works because new Date(dateObj) handles both, but any future arithmetic on timestamp would produce wrong results. Consider normalizing to number: clone['timestamp'] = typeof ts === 'string' ? new Date(ts).getTime() : ts;
— qwen3.7-max via Qwen Code /review
| <> | ||
| {showTimestamps && itemForDisplay.timestamp != null && ( | ||
| <Text dimColor> | ||
| [ |
There was a problem hiding this comment.
[Suggestion] toLocaleTimeString('en-US', { hour12: false }) doesn't guarantee the HH:MM:SS format across all Node.js/ICU builds. Some ICU versions produce 9:05:30 instead of 09:05:30 for single-digit hours.
The codebase already has the correct pattern at demo.ts:202:
| [ | |
| {new Date(itemForDisplay.timestamp).toLocaleTimeString('en-US', { | |
| hour12: false, | |
| hour: '2-digit', | |
| minute: '2-digit', | |
| second: '2-digit', | |
| })} |
— qwen3.7-max via Qwen Code /review
| { settings: makeTimestampSettings() }, | ||
| ); | ||
| expect(lastFrame()).toMatch(/\[\d{2}:\d{2}:\d{2}\]/); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] The test fixture hardcodes timestamp: new Date('2026-01-15T14:30:45').getTime() but the regex \[\d{2}:\d{2}:\d{2}\] only asserts format, not value. A bug rendering the wrong time (e.g., Date.now() instead of item.timestamp) would pass undetected.
Pin the assertion to the expected value:
// Using UTC timestamp for deterministic assertion:
expect(lastFrame()).toContain('[14:30:45]');
// Or derive from fixture:
const expected = new Date('2026-01-15T14:30:45').toLocaleTimeString('en-US', { hour12: false });
expect(lastFrame()).toContain(`[${expected}]`);— qwen3.7-max via Qwen Code /review
| export type HistoryItemGemini = HistoryItemBase & { | ||
| type: 'gemini'; | ||
| text: string; | ||
| timestamp?: number; |
There was a problem hiding this comment.
[Suggestion] Placing timestamp?: number only on HistoryItemGemini forces 'timestamp' in item checks and as HistoryItemGemini casts in the setPendingHistoryItem updater and split path. Moving it to HistoryItemBase would simplify those call sites — the updater becomes { type, text, timestamp: item?.timestamp } with no casts.
— qwen3.7-max via Qwen Code /review
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. The earlier timestamp-overwrite and TS2345 fixes hold up, the conflict is resolved, and the current head passes the full CI matrix (lint + tests on all three OS + CodeQL).
|
@copilot resolve the merge conflicts in this pull request |
b697240
b697240 to
bbf0095
Compare
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reflection
Stepping back: this PR does one thing well — adds optional [HH:MM:SS] timestamps before assistant turns. The implementation matches what I'd have done independently. The commitItem wrapper is the right level of indirection, the setting is opt-in with a sensible default, and the tmux tests confirm it works in both directions.
The one issue from the prior triage has been corrected: the SessionPreview.test.tsx migration is necessary (not drive-by refactoring), but it's incomplete — one test was missed and now fails. The fix is a one-line change: wrap that test in renderWithProviders instead of render + KeypressProvider.
The merge conflicts that blocked the prior triage are now resolved. The PR is MERGEABLE from a git perspective, just needs this test fix.
Verdict: Request changes — fix the one unmigrated test in SessionPreview.test.tsx (line 116–137), then this is ready to ship. @ZijianZhang989
中文说明
总结
退一步看：这个 PR 把一件事做好了——在助手 turn 前添加可选的 [HH:MM:SS] 时间戳。实现方式跟我的独立方案一致。commitItem 包装器是正确层次的间接调用，设置项是 opt-in 且有合理默认值，tmux 测试确认两个方向都能正常工作。
上次 triage 的一个判断已纠正：SessionPreview.test.tsx 迁移是必要的（不是顺手重构），但不完整——有一个测试被遗漏了，现在会失败。修复很简单：把那个测试用 renderWithProviders 包装替代 render + KeypressProvider。
上次 triage 的合并冲突已经解决。从 git 角度看 PR 已经可以合并 (MERGEABLE)，只需要修复这个测试。
结论： 请求修改——修复 SessionPreview.test.tsx 中一个未迁移的测试（第 116–137 行），然后就可以合入了。@ZijianZhang989
— Qwen Code · qwen3.7-max
6cf50a5 to
78555df
Compare
78555df to
076071e
Compare
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No new review findings on this pass — the split-path timestamp propagation concern was already captured in prior bot comments on this PR. Downgraded from Approve to Comment: CI still running. Two additional low-confidence observations surfaced for human review: (1) useGeminiStream.test.tsx has no positive test verifying commitItem attaches a timestamp to gemini items (assertions were relaxed to expect.objectContaining without a corresponding positive assertion), and (2) convertToHistoryItems in resumeHistoryUtils.ts omits timestamp on resumed gemini items even though record.timestamp is in scope — resumed sessions will silently show no timestamps on historical turns when showTimestamps is enabled. Lint, typecheck, and 159/159 tests pass locally. — qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM — previous blocker (unmigrated SessionPreview test) is resolved, all tests pass (7/7 SessionPreview, 24/24 HistoryItemDisplay), and tmux interactive testing confirms timestamps render correctly when enabled and are absent when disabled. Ready to ship. ✅
5 participants
What this PR does
Adds an optional
output.showTimestampssetting that renders a[HH:MM:SS]timestamp before each assistant response in the CLI. The timestamp appears immediately when the model starts outputting, not after the response completes. The implementation uses acommitItemwrapper aroundaddIteminuseGeminiStreamto attachDate.now()togeminitype items, including the split-code path that previously bypassed timestamp assignment. Only the first item of each assistant turn gets a timestamp;gemini_contentitems (performance-split continuations) do not, preventing duplicate timestamps within the same turn.Why it's needed
Users requested the ability to see when each assistant response was generated, similar to Claude Code's turn duration display. This helps with debugging, logging, and understanding the temporal flow of conversations. The feature is opt-in (default off) to avoid cluttering the UI for users who don't need it.
Reviewer Test Plan
How to verify
npm run dev/settingsoutput→showTimestampsand enable it[HH:MM:SS]appear immediately when the model starts respondingshowTimestampsin settings — next response should not have timestamp, no restart neededEvidence (Before & After)
Will provide video recording.
Tested on
Environment (optional)
Tested with
npm run devon macOS 14.5, Node.js v22.Risk & Scope
commitItemwrapper adds minimal overhead (~1 function call per item commit).false, so existing users see no change. Settings are automatically persisted to~/.qwen/settings.json.Linked Issues
Refs #4899
中文说明
这个 PR 做了什么
添加了一个可选的
output.showTimestamps设置，在 CLI 的每个助手回复前渲染一个[HH:MM:SS]时间戳。时间戳在模型开始输出时立即显示，而不是在响应完成后才显示。实现在useGeminiStream中使用commitItem包装器来包装addItem，将Date.now()附加到gemini类型的 item 上，包括之前会绕过时间戳分配的 split 代码路径。只有每个助手 turn 的第一个 item 会获得时间戳；gemini_contentitems（性能分割的延续）不会，防止同一个 turn 内出现重复的时间戳。为什么需要它
用户希望看到每个助手响应的生成时间，类似于 Claude Code 的 turn duration 显示。这有助于调试、记录日志和理解对话的时间流动。该功能是可选的（默认关闭），避免给不需要的用户造成 UI 混乱。
审阅者测试计划
如何验证
/settingsoutput→showTimestamps并启用它[HH:MM:SS]在模型开始响应时立即出现showTimestamps— 下次回复时间戳应该消失，无需重启证据（前后对比）
见视频
已测试平台
环境（可选）
在 macOS 14.5, Node.js v22 测试。
风险与范围
commitItem包装器增加了最小的开销（每个 item 提交约 1 次函数调用）。false，所以现有用户看不到任何变化。设置会自动持久化到~/.qwen/settings.json。关联的 Issues
Refs #4899