fix(cli): /status preserves prior error history items (#4169)#4265
Merged
Conversation
The guard at the top of `submitQuery` cleared `pendingRetryErrorItem`
via `clearRetryCountdown()` without first persisting it to history, so
any new user turn — including informational slash commands like
`/status`, `/about`, or `/help` — would silently discard a visible API
error. The user could not refer back to the failure they were
investigating.
The inline comment already described the intended behavior ("Commit any
pending retry error to history (without hint) since the user is starting
a new conversation turn") but the commit step was missing. Add the
`addItem` call before clearing, stripping the now-stale "Press Ctrl+Y to
retry" hint so the persisted history entry reads cleanly.
Fixes #4169
Contributor
📋 Review SummaryThis PR fixes a bug where running 🔍 General Feedback
🎯 Specific Feedback🔵 Low
function commitPendingErrorToHistory(
pendingError: HistoryItemError | null,
addItem: (item: HistoryItemWithoutId, timestamp: number) => void,
timestamp: number
) {
if (pendingError && pendingError.type === 'error') {
const { hint: _hint, ...errorWithoutHint } = pendingError;
addItem(errorWithoutHint, timestamp);
}
}This would make the intent at the call site even clearer and allow unit testing this logic in isolation. However, the current inline implementation is still readable and correct.
const errorCommit = mockAddItem.mock.calls.find(
([item]) => item?.type === 'error',
);
expect(errorCommit?.[0]).not.toHaveProperty('hint');Could become: expect(mockAddItem).toHaveBeenCalledWith(
expect.objectContaining({ type: 'error', hint: undefined }),
expect.any(Number),
);This is a minor style preference; the current test works correctly. ✅ Highlights
|
Contributor
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. |
wenshao
reviewed
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Review — PR #4265
Overview
Surgical fix for #4169: when a new turn starts (including informational slash commands like /status), commit the pending retry error to persistent history BEFORE clearing it, instead of silently discarding it. The clearRetryCountdown() call at useGeminiStream.ts:1696 previously dropped pendingRetryErrorItemRef.current on the floor; the new lines 1691-1695 splice an addItem(errorWithoutHint, userMessageTimestamp) in ahead of the clear, with the now-stale "Press Ctrl+Y to retry" hint stripped.
Correctness — looks solid
- The
hintdestructure is type-safe:HistoryItemErrordeclareshint?: stringinpackages/cli/src/ui/types.ts, so the rest-spread produces a validHistoryItemErrorshape. - The discriminator narrow (
pendingError.type === 'error') is defensive — the underlying ref isHistoryItemWithoutId | null, and the three set-sites (useGeminiStream.ts:486-490,:1091-1095,:1880-1887) all produce error-shaped items, but the guard keeps it robust if a future set-site lands a different variant. - Order is right:
addItemruns beforeclearRetryCountdown()so the persisted entry exists by the timesetPendingRetryErrorItem(null)fires. - The outer guard
pendingRetryCountdownItemRef.current || pendingRetryErrorItemRef.currentis unchanged — countdown-only-without-error still goes throughclearRetryCountdownwith no commit, which is correct (the countdown is a transient timer display, not persistent UX). - Idempotency holds: on a subsequent
submitQuery,pendingRetryErrorItemRef.currentis alreadynull, so the inneraddItemis skipped — no double-commit.
Tests
- New regression test
commits pending retry error to history (without hint) when a new query startsexercises the full path: error → first submit produces pending item → second submit commits to history, stripshint, drains pending region. .not.toHaveProperty('hint')is a precise assertion; verifies the stale "Press Ctrl+Y to retry" doesn't leak into persistent history.- Existing
should clear static error when starting a new querystill passes (per PR notes — pending area still drains). - The test sits in the same describe block as the existing static-error-clearing test; well placed for future maintainers reading either path.
Minor observation (non-blocking)
Timestamp semantics: the committed error uses userMessageTimestamp (= Date.now() at the START of the new turn), not the timestamp of the original failed turn. That means after the fix, the error appears in history with the SAME timestamp as the user's /status invocation. Visually correct (error renders just before the /status line), and it's the only viable choice since the pending ref doesn't carry the original timestamp. Worth a one-line comment near the addItem call so future maintainers don't try to "fix" the timestamp drift, but not blocking.
CI / risk
- All checks green: Lint, CodeQL, Test (ubuntu/macos/windows Node 22.x), Classify PR, review-pr.
- Diff is 73 additions / 0 deletions — purely additive on the cleanup path. Revert is a one-block delete.
- No new dependencies, no API surface change, no config flag.
Verdict
APPROVE. Clean root-cause fix with a tight regression test. Ready to merge once the maintainers are around. The hint-stripping detail (committed history line stays readable without the stale "Ctrl+Y" prompt) shows the author thought past the bare-minimum patch.
wenshao
reviewed
May 18, 2026
| const pendingError = pendingRetryErrorItemRef.current; | ||
| if (pendingError && pendingError.type === 'error') { | ||
| const { hint: _hint, ...errorWithoutHint } = pendingError; | ||
| addItem(errorWithoutHint, userMessageTimestamp); |
Collaborator
There was a problem hiding this comment.
Nit — timestamp semantics worth a one-liner.
userMessageTimestamp here is Date.now() from line 1670, i.e. the START of the new turn — not when the original API failure actually happened. That's fine and probably the only viable choice (the pending ref doesn't carry the original timestamp), but at a glance a future maintainer might read this as a bug and try to "fix" the drift.
Suggest a short comment so the intent is explicit:
Suggested change
| addItem(errorWithoutHint, userMessageTimestamp); | |
| // Reuse the new turn's timestamp — the pending ref doesn't | |
| // carry the original failure time, and this keeps the | |
| // committed error visually adjacent to the user input that | |
| // displaced it (e.g. `/status`). | |
| addItem(errorWithoutHint, userMessageTimestamp); |
| pendingRetryErrorItemRef.current | ||
| ) { | ||
| const pendingError = pendingRetryErrorItemRef.current; | ||
| if (pendingError && pendingError.type === 'error') { |
Collaborator
There was a problem hiding this comment.
Defensive narrow looks right. pendingRetryErrorItemRef is typed HistoryItemWithoutId | null, and the three set-sites (:486-490, :1091-1095, :1880-1887) all produce type: 'error' items today — but the discriminator guard keeps this robust if a future set-site lands a different variant, and HistoryItemError declares hint?: string in types.ts so the rest-spread is type-safe with no hint property in the persisted shape. No change requested; flagging because it's a thoughtful detail.
wenshao
approved these changes
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
Collaborator
Local verification reportBuilt both the PR head ( End-to-end (tmux + mock 400 server)
Pre-fix capture (bug reproduces)The Post-fix capture (fixed)Error remains in the persistent Automated checks (local)
AssessmentDiff is 5 lines of implementation + 68 lines of test. The fix realizes intent that the existing inline comment already described ( LGTM — ready to merge. |
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
submitQuery(lines 1683-1692 ofuseGeminiStream.ts) clearedpendingRetryErrorItemviaclearRetryCountdown()without committing it to history first. Any new user turn — including informational slash commands like/status,/about,/help— silently discarded a visible API error.addItem()before clearing, stripping the now-stale "Press Ctrl+Y to retry" hint so the persisted history entry reads cleanly.Why this matters
Reporter triggered an API error (
reasoning_contentfrom DeepSeek V4 Pro thinking-mode), then ran/statusto grab their version info for the bug report. The status box rendered, but the error message vanished entirely — neither in the persistent (<Static>) history nor in the dynamic region. They lost the context they were investigating.Note: this PR fixes the UI clobber only. The underlying
reasoning_contentAPI error is tracked separately under #3772 and is out of scope here.Test plan
commits pending retry error to history (without hint) when a new query startsin useGeminiStream.test.tsx — asserts the error is committed to history with the hint stripped, then cleared from the pending areashould clear static error when starting a new querystill passes (pending area still drains)packages/cli/src/ui/hooks/useGeminiStream.test.tsx— 95/95 passingnpm run typecheck— cleannpm run lint— clean/status; after the fix the error remains visible in the<Static>history above the about box, with the retry hint correctly omittedAuthDialog/SettingsDialogfailures (verified to fail onmainbaseline without my changes) — unrelated to this fixFixes #4169