fix: resolve 4 bugs in termui - #3612
Conversation
github-actions
Bot
added
type:bug
📝 WalkthroughWalkthroughThe changes update interval cleanup, whitespace-only paragraph parsing, Promise rejection logging, and floating-point rounding in switch animation positioning. ChangesExample behavior fixes
UI behavior fixes
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.6)packages/ui/src/Form.tsFile contains syntax errors that prevent linting: Line 142: Expected a statement but instead found '.catch(err => console.error("Promise.all failed:", err))'. 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)
packages/ui/src/Switch.ts (1)
117-117: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for the rounding boundaries.
The existing test only verifies that rendering stays within the widget width. Add an assertion for knob positions near the
0.5and1.5rounding thresholds to verify theNumber.EPSILONfix.🤖 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 `@packages/ui/src/Switch.ts` at line 117, Add a regression test covering the rounding boundaries used by knobPos in Switch rendering: verify values just below and above 0.5 and 1.5 produce the expected rounded knob positions, including the Number.EPSILON adjustment. Keep the existing width-boundary assertion and target the test for the Switch rendering behavior.
🤖 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 `@packages/ui/src/Form.ts`:
- Around line 141-142: Fix the validation flow around Promise.all so the
rejection handler is attached to await Promise.all(validationPromises) rather
than left as a standalone .catch expression. On rejection, reset _isValidating,
call markDirty(), and return before processing results; remove the detached
handler so Form.ts parses correctly.
---
Nitpick comments:
In `@packages/ui/src/Switch.ts`:
- Line 117: Add a regression test covering the rounding boundaries used by
knobPos in Switch rendering: verify values just below and above 0.5 and 1.5
produce the expected rounded knob positions, including the Number.EPSILON
adjustment. Keep the existing width-boundary assertion and target the test for
the Switch rendering behavior.
🪄 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: ec1cefb4-63bc-4cfd-93ea-b30abbb34e6e
📒 Files selected for processing (4)
examples/ai-streaming/src/index.tsxexamples/chat-app/src/index.tsxpackages/ui/src/Form.tspackages/ui/src/Switch.ts
|
|
||
| .catch(err => console.error("Promise.all failed:", err)); No newline at end of file |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Attach the rejection handler to Promise.all.
Line 142 is a standalone .catch(...) expression, so the file does not parse. The handler is also detached from the Promise.all call at Line 83.
Move the error handling to a try/catch around await Promise.all(validationPromises) or chain it directly to that promise. On rejection, reset _isValidating, call markDirty(), and return before processing results.
🧰 Tools
🪛 Biome (2.5.6)
[error] 142-142: Expected a statement but instead found '.catch(err => console.error("Promise.all failed:", err))'.
(parse)
🪛 GitHub Actions: CI / 0_build-and-test.txt
[error] 142-142: @termuijs/ui build failed during tsup/esbuild and TypeScript declaration generation. Unexpected '.' at the start of '.catch(err => console.error("Promise.all failed:", err));', causing syntax errors and exit code 1.
🪛 GitHub Actions: CI / build-and-test
[error] 142-142: The @termuijs/ui build failed during tsup/esbuild parsing due to an unexpected '.' at the start of '.catch(err => console.error("Promise.all failed:", err));'. TypeScript also reported declaration/statement and syntax errors on this line.
🤖 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 `@packages/ui/src/Form.ts` around lines 141 - 142, Fix the validation flow
around Promise.all so the rejection handler is attached to await
Promise.all(validationPromises) rather than left as a standalone .catch
expression. On rejection, reset _isValidating, call markDirty(), and return
before processing results; remove the detached handler so Form.ts parses
correctly.
Source: Linters/SAST tools
1 participant
Description
This PR fixes real bugs found in the codebase:
trim()to''misses whitespace-only input;.trim().length === 0is explicit.Promise.all: an unhandled rejection in any input promise previously crashed silently.Number.EPSILONtoMath.round: prevents floating-point drift (e.g.1.005 * 100rounding to 100 instead of 101).Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3611
Summary by CodeRabbit