fix: resolve 4 bugs in termui - #3540
Conversation
📝 WalkthroughWalkthroughThe changes update three validation and timer checks: streaming interval cleanup, whitespace-only line detection, and parsed-number validation. ChangesStreaming timer cleanup
Chat block parsing
Prompt selection validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@examples/ai-streaming/src/index.tsx`:
- Around line 43-45: Update AIStreamingApp’s streaming timer setup to store the
interval handle in a private instance field instead of the shared
window.__interval, so each instance owns its timer. Extend cleanup() to clear
that interval and release the handle, preventing tick() after unmount and
allowing multiple instances to run independently.
🪄 Autofix
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: a09ad106-c4fa-4497-983b-04694a07795b
📒 Files selected for processing (3)
examples/ai-streaming/src/index.tsxexamples/chat-app/src/index.tsxpackages/ui/src/prompts.ts
| clearInterval(window.__interval); window.__interval = setInterval(() => { | ||
| this._streamingText.tick(); | ||
| }, 50); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Give the streaming interval instance ownership and teardown.
window.__interval is shared by all AIStreamingApp instances. Constructing a second instance clears the first instance's timer. The class also does not clear the interval when the widget is unmounted, so the callback can continue calling tick() on a detached widget and retain the instance.
Store the handle in a private field and clear it from cleanup(), as shown by examples/widget-gallery/src/tabs/ai-tab.ts’s cleanup() implementation.
Proposed fix
+ private _streamInterval: ReturnType<typeof setInterval>;
+
constructor() {
...
- clearInterval(window.__interval); window.__interval = setInterval(() => {
+ this._streamInterval = setInterval(() => {
this._streamingText.tick();
}, 50);
}
+
+ cleanup(): void {
+ clearInterval(this._streamInterval);
+ }📝 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.
| clearInterval(window.__interval); window.__interval = setInterval(() => { | |
| this._streamingText.tick(); | |
| }, 50); | |
| private _streamInterval: ReturnType<typeof setInterval>; | |
| constructor() { | |
| this._streamInterval = setInterval(() => { | |
| this._streamingText.tick(); | |
| }, 50); | |
| } | |
| cleanup(): void { | |
| clearInterval(this._streamInterval); | |
| } |
🤖 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 `@examples/ai-streaming/src/index.tsx` around lines 43 - 45, Update
AIStreamingApp’s streaming timer setup to store the interval handle in a private
instance field instead of the shared window.__interval, so each instance owns
its timer. Extend cleanup() to clear that interval and release the handle,
preventing tick() after unmount and allowing multiple instances to run
independently.
Description
This PR fixes real bugs found in the codebase:
trim()to''misses whitespace-only input;.trim().length === 0is explicit.x === trueis equivalent tox(andx === falseto!x), and shorter to read.isNaNwithNumber.isNaN: the global version coerces its argument, soisNaN('1')returns false whileNumber.isNaNis strict.Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3539
Summary by CodeRabbit