fix: resolve 4 bugs in termui - #3541
Conversation
github-actions
Bot
added
type:bug
📝 WalkthroughWalkthroughThe changes apply four targeted correctness fixes: more precise percentage rounding, explicit decimal tab parsing, numeric sorting of selected options, and logging for rejected form operations. ChangesCorrectness fixes
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 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: 2
🤖 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/pomodoro-timer/src/index.tsx`:
- Line 185: Update the percentage calculation in the label expression within the
timer rendering method so Number.EPSILON is applied to this._value before
multiplying by 100, ensuring values near rounding boundaries display the correct
percentage while preserving the existing conditional label behavior.
In `@packages/ui/src/Form.ts`:
- Around line 141-142: Fix the Promise.all flow in Form validation by attaching
the rejection handler directly to the Promise.all call near the existing
validation logic, or wrap it in try/catch. On rejection, reset _isValidating,
call markDirty(), and return before invoking _onSubmit; do not allow submission
to continue with an empty result.
🪄 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: c4940d80-a89d-425b-ba50-de3ede733e59
📒 Files selected for processing (4)
examples/pomodoro-timer/src/index.tsxexamples/showcase/src/index.tsxpackages/ui/src/Form.tspackages/ui/src/MultiSelect.ts
| const attrs = styleToCellAttrs(this._style); | ||
|
|
||
| const label = this._showLabel ? ` ${Math.round(this._value * 100)}%` : ''; | ||
| const label = this._showLabel ? ` ${Math.round(this._value * 100 + Number.EPSILON)}%` : ''; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Apply the epsilon before scaling the value.
Number.EPSILON is added after multiplication by 100. At this magnitude, it can have no effect. A value near 0.285 can still display 28% instead of 29%.
Apply the epsilon before multiplication, or scale it for the percentage calculation.
Proposed fix
- const label = this._showLabel ? ` ${Math.round(this._value * 100 + Number.EPSILON)}%` : '';
+ const label = this._showLabel ? ` ${Math.round((this._value + Number.EPSILON) * 100)}%` : '';📝 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.
| const label = this._showLabel ? ` ${Math.round(this._value * 100 + Number.EPSILON)}%` : ''; | |
| const label = this._showLabel ? ` ${Math.round((this._value + Number.EPSILON) * 100)}%` : ''; |
🤖 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/pomodoro-timer/src/index.tsx` at line 185, Update the percentage
calculation in the label expression within the timer rendering method so
Number.EPSILON is applied to this._value before multiplying by 100, ensuring
values near rounding boundaries display the correct percentage while preserving
the existing conditional label behavior.
|
|
||
| .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.
The standalone .catch(...) at Line 142 is not attached to an expression. Biome reports a parse error, so the package cannot build.
Attach the handler to the Promise.all call at Line 83, or use try/catch. On rejection, reset _isValidating, call markDirty(), and return before _onSubmit. Do not convert the rejection into an empty result that allows submission to continue.
🧰 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: The @termuijs/ui build failed during tsup/esbuild because of an unexpected '.' in .catch(err => console.error("Promise.all failed:", err));. TypeScript also reported syntax errors including TS1128, TS1005, and TS2304.
🪛 GitHub Actions: CI / build-and-test
[error] 142-142: The @termuijs/ui build failed during tsup/esbuild: unexpected '.' at '.catch(err => console.error("Promise.all failed:", err));'. TypeScript also reported TS1128, TS1005, and TS2304 syntax errors.
🤖 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 Promise.all flow in
Form validation by attaching the rejection handler directly to the Promise.all
call near the existing validation logic, or wrap it in try/catch. On rejection,
reset _isValidating, call markDirty(), and return before invoking _onSubmit; do
not allow submission to continue with an empty result.
Source: Linters/SAST tools
1 participant
Description
This PR fixes real bugs found in the codebase:
Number.EPSILONtoMath.round: prevents floating-point drift (e.g.1.005 * 100rounding to 100 instead of 101)..sort()coerces elements to strings, so[10, 9, 2]sorts as[10, 2, 9]; numeric comparator sorts correctly.Promise.all: an unhandled rejection in any input promise previously crashed silently.parseInt: without10, strings like'0x1F'or'08'parse in unintended bases.Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3540
Summary by CodeRabbit