fix(webview): anchor thinking timer to message timestamp to survive Virtuoso remounts#881
fix(webview): anchor thinking timer to message timestamp to survive Virtuoso remounts#881umi008 wants to merge 6 commits into
Conversation
…emounts The ReasoningBlock previously anchored its elapsed-time display to Date.now() at component mount time. When Virtuoso's virtual list recycles or remounts the component (e.g. during expand/collapse toggles or scroll), the timer reset to 0 because a fresh Date.now() was used and the effect guard (isLast && isStreaming) was false on remount. Now the component uses the message creation timestamp (ts, already passed as a prop but previously discarded) as the timer anchor. On remount after streaming has finished, elapsed is initialized from Date.now() - ts, giving a reasonable approximation of the thinking duration instead of showing 0s. This also explains the zh_CN-specific reproduction: CJK character widths in the header created slightly different DOM geometry, pushing the layout past Virtuoso's recycle threshold in certain viewports. Fixes Zoo-Code-Org#656
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesReasoning timer persistence
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webview-ui/src/components/chat/ReasoningBlock.tsx (1)
17-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Vitest coverage for the ReasoningBlock remount timer path.
Add a local test underwebview-ui/src/components/chat/__tests__that unmounts and remounts the component with the sametsand asserts the elapsed label still reflects the original message timestamp.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/components/chat/ReasoningBlock.tsx` around lines 17 - 29, Add a Vitest test under the chat component tests covering ReasoningBlock remount behavior: render it with a fixed ts, advance mocked time, unmount and remount using the same ts, then assert the elapsed label reflects time since the original timestamp rather than resetting on mount. Use the existing ReasoningBlock render props and translation/state mocks as needed.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webview-ui/src/components/chat/ReasoningBlock.tsx`:
- Line 29: Initialize the elapsed state in ReasoningBlock directly from
Math.max(0, Date.now() - ts) for both streaming and non-streaming states,
removing the isStreaming conditional so remounts preserve the current elapsed
time and avoid the temporary hidden label.
---
Nitpick comments:
In `@webview-ui/src/components/chat/ReasoningBlock.tsx`:
- Around line 17-29: Add a Vitest test under the chat component tests covering
ReasoningBlock remount behavior: render it with a fixed ts, advance mocked time,
unmount and remount using the same ts, then assert the elapsed label reflects
time since the original timestamp rather than resetting on mount. Use the
existing ReasoningBlock render props and translation/state mocks as needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d92d708-5c96-4b59-ac23-c94d8d20b1f5
📒 Files selected for processing (1)
webview-ui/src/components/chat/ReasoningBlock.tsx
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
the
awaiting-review
Covers the ts-anchored elapsed timer behavior including: - Non-streaming elapsed anchored to message timestamp - Streaming timer that ticks with real time - Remount survival (Virtuoso recycle) — elapsed anchored to ts, not mount - Timer stops when streaming ends - Collapse behavior - Zero-elapsed label hiding
There was a problem hiding this comment.
🧹 Nitpick comments (1)
webview-ui/src/components/chat/__tests__/ReasoningBlock.spec.tsx (1)
132-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset
mockExtensionStateinbeforeEachinstead of manually at end of test.If the assertion on line 138 fails, the reset on line 141 never executes, and subsequent tests inherit
reasoningBlockCollapsed: true. Moving the reset tobeforeEachguarantees a clean slate per test regardless of pass/fail outcomes.♻️ Proposed fix
beforeEach(() => { vi.useFakeTimers() vi.setSystemTime(Date.now()) + mockExtensionState.reasoningBlockCollapsed = false })Then remove the manual reset:
// Reset for other tests - mockExtensionState.reasoningBlockCollapsed = false })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/components/chat/__tests__/ReasoningBlock.spec.tsx` around lines 132 - 142, Reset mockExtensionState.reasoningBlockCollapsed in the test suite’s beforeEach setup so every ReasoningBlock test starts with the default state, even when an assertion fails. Remove the manual reset from the “collapses when reasoningBlockCollapsed is true” test while preserving its test-specific assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@webview-ui/src/components/chat/__tests__/ReasoningBlock.spec.tsx`:
- Around line 132-142: Reset mockExtensionState.reasoningBlockCollapsed in the
test suite’s beforeEach setup so every ReasoningBlock test starts with the
default state, even when an assertion fails. Remove the manual reset from the
“collapses when reasoningBlockCollapsed is true” test while preserving its
test-specific assignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 37f2f27c-3b06-4d70-9db0-e3f701ab821e
📒 Files selected for processing (1)
webview-ui/src/components/chat/__tests__/ReasoningBlock.spec.tsx
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Thanks for your contribution, had a question of the state mechanics around keeping track of the timer value.
…dule cache Adds a module-level Map<number, number> that caches the final elapsed reasoning duration keyed by message timestamp (ts). This solves the edge case where a remount minutes after streaming finished would recompute Date.now() - ts and show an inflated elapsed time. - elapsedCache survives React unmount/remount cycles since module state persists for the page lifetime - Cache is populated on each timer tick while streaming - Cache is also populated when streaming changes to false - On component init, cache is checked first before any computation - Map size is bounded by the number of reasoning blocks per session
|
addressed @edelauna review feedback: added a module-level elapsedCache (Map<number, number>) that persists the final thinking duration across React remounts. on late remounts (scrolled away for minutes, history reopened), the cached value is returned instead of recomputing Date.now() - ts, which would give an inflated elapsed time. commit f957b59 on fix/656-reasoning-timer-reset |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webview-ui/src/components/chat/ReasoningBlock.tsx (1)
33-38: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove potentially stale
useRefcache forts.
startTimeRefcapturestson the first render but will not update if the component is recycled with new props.tsshould be used directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/components/chat/ReasoningBlock.tsx` around lines 33 - 38, Remove the startTimeRef useRef cache in ReasoningBlock and use the current ts prop directly wherever the elapsed timer calculates its start time. Ensure recycled or remounted components always reflect the latest message creation timestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webview-ui/src/components/chat/ReasoningBlock.tsx`:
- Around line 53-64: Update ReasoningBlock’s start-time handling by removing the
redundant startTimeRef initialization, then make the interval tick in the
useEffect calculate elapsed time from the current ts prop and include ts in the
effect dependency array. Apply these changes at
webview-ui/src/components/chat/ReasoningBlock.tsx:53-64 and remove the obsolete
ref usage at webview-ui/src/components/chat/ReasoningBlock.tsx:33-38.
---
Outside diff comments:
In `@webview-ui/src/components/chat/ReasoningBlock.tsx`:
- Around line 33-38: Remove the startTimeRef useRef cache in ReasoningBlock and
use the current ts prop directly wherever the elapsed timer calculates its start
time. Ensure recycled or remounted components always reflect the latest message
creation timestamp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 406324c3-22ab-49b8-89bf-5e6b5c983694
📒 Files selected for processing (2)
webview-ui/src/components/chat/ReasoningBlock.tsxwebview-ui/src/components/chat/__tests__/ReasoningBlock.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- webview-ui/src/components/chat/tests/ReasoningBlock.spec.tsx
| useEffect(() => { | ||
| if (isLast && isStreaming) { | ||
| const tick = () => setElapsed(Date.now() - startTimeRef.current) | ||
| const tick = () => { | ||
| const current = Date.now() - startTimeRef.current | ||
| setElapsed(current) | ||
| elapsedCache.set(ts, current) | ||
| } | ||
| tick() | ||
| const id = setInterval(tick, 1000) | ||
| return () => clearInterval(id) | ||
| } | ||
| }, [isLast, isStreaming]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Stale closure and improper reactivity for ts prop.
The interval logic bypasses React's reactivity rules, which will cause bugs if Virtuoso recycles the component with new props (a new ts):
webview-ui/src/components/chat/ReasoningBlock.tsx#L53-L64:tsis omitted from the dependency array, causingtickto capture a stale closure. Update the calculation to usetsdirectly and addtsto the dependency array.webview-ui/src/components/chat/ReasoningBlock.tsx#L33-L38:startTimeRefcapturestsonly on the initial mount and fails to update. Remove this redundant ref.
🛠️ Proposed fix
Remove the redundant ref:
- // Anchor the elapsed timer to the message creation timestamp (ts)
- // rather than component mount time. When Virtuoso recycles or
- // remounts this component (e.g. during expand/collapse in a
- // virtualized list), the timer survives because ts is a stable
- // prop from the message data rather than a fresh Date.now().
- const startTimeRef = useRef<number>(ts)Update the effect to use ts directly and declare it as a dependency:
useEffect(() => {
if (isLast && isStreaming) {
const tick = () => {
- const current = Date.now() - startTimeRef.current
+ const current = Date.now() - ts
setElapsed(current)
elapsedCache.set(ts, current)
}
tick()
const id = setInterval(tick, 1000)
return () => clearInterval(id)
}
- }, [isLast, isStreaming])
+ }, [isLast, isStreaming, ts])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (isLast && isStreaming) { | |
| const tick = () => setElapsed(Date.now() - startTimeRef.current) | |
| const tick = () => { | |
| const current = Date.now() - startTimeRef.current | |
| setElapsed(current) | |
| elapsedCache.set(ts, current) | |
| } | |
| tick() | |
| const id = setInterval(tick, 1000) | |
| return () => clearInterval(id) | |
| } | |
| }, [isLast, isStreaming]) | |
| - // Anchor the elapsed timer to the message creation timestamp (ts) | |
| - // rather than component mount time. When Virtuoso recycles or | |
| - // remounts this component (e.g. during expand/collapse in a | |
| - // virtualized list), the timer survives because ts is a stable | |
| - // prop from the message data rather than a fresh Date.now(). | |
| - const startTimeRef = useRef<number>(ts) | |
| - | |
| useEffect(() => { | |
| if (isLast && isStreaming) { | |
| const tick = () => { | |
| const current = Date.now() - ts | |
| setElapsed(current) | |
| elapsedCache.set(ts, current) | |
| } | |
| tick() | |
| const id = setInterval(tick, 1000) | |
| return () => clearInterval(id) | |
| } | |
| }, [isLast, isStreaming, ts]) |
📍 Affects 1 file
webview-ui/src/components/chat/ReasoningBlock.tsx#L53-L64(this comment)webview-ui/src/components/chat/ReasoningBlock.tsx#L33-L38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webview-ui/src/components/chat/ReasoningBlock.tsx` around lines 53 - 64,
Update ReasoningBlock’s start-time handling by removing the redundant
startTimeRef initialization, then make the interval tick in the useEffect
calculate elapsed time from the current ts prop and include ts in the effect
dependency array. Apply these changes at
webview-ui/src/components/chat/ReasoningBlock.tsx:53-64 and remove the obsolete
ref usage at webview-ui/src/components/chat/ReasoningBlock.tsx:33-38.
the timer effect closes over ts (for elapsedCache.set) but was missing it from the dependency array, triggering react-hooks/exhaustive-deps
github-actions
Bot
added
awaiting-author
2 participants
Related GitHub Issue
Closes: #656
Description
The thinking/reasoning timer (e.g., "Thought for 30s") was resetting to 0 or disappearing when the user expanded and then collapsed the thinking section. This happened because
ReasoningBlockusedDate.now()as the timer anchor at component mount time — but inside aVirtuosovirtual list, expand/collapse can trigger DOM recycling that unmounts and remounts the component. On remount,startTimeRefgets a freshDate.now(), and sinceisStreamingis alreadyfalse(the message finished streaming before the user interacted), the timer effect never runs — leavingelapsedat 0.This was more visible with zh_CN locale because CJK character widths in the header text produced different DOM geometry, pushing the layout past Virtuoso recycle thresholds in viewports where Latin text would not trigger a remount.
Fix: The component now uses the message creation timestamp (
tsprop, already passed fromChatRow.tsxbut previously discarded in the destructuring) as the timer anchor. On remount after streaming has finished,elapsedis initialized fromDate.now() - ts, giving a reasonable approximation of the thinking duration instead of 0.Test Procedure
Pre-Submission Checklist
ts; component just wasn't using it — this is a pure hook into existing dataScreenshots / Videos
N/A — state management fix, no visual change beyond the timer not resetting.
Documentation Updates
Additional Notes
Edge case: for messages scrolled away for minutes and then scrolled back,
Date.now() - tswill show longer than the actual thinking time. This is cosmetic and far preferable to showing 0 or hiding the timer entirely — and only affects the unlikely case of a late remount after significant wall-clock time.Summary by CodeRabbit