fix: resolve 4 bugs in termui - #3561
Conversation
github-actions
Bot
added
type:bug
📝 WalkthroughWalkthroughThe pull request updates percentage rounding in two examples, adds rejection logging to a form Promise.all operation, and applies numeric sorting to selected MultiSelect option indexes. 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: Move Number.EPSILON before percentage scaling in both renderers:
update the percentage calculation in examples/pomodoro-timer/src/index.tsx at
lines 185-185 and the corresponding calculation in
examples/todo-app/src/index.ts at lines 107-107 to round (value +
Number.EPSILON) after adding epsilon to the normalized value, preserving the
existing conditional label behavior.
In `@packages/ui/src/Form.ts`:
- Around line 141-142: Fix the validation flow around the Promise.all call so
its rejection handler is syntactically attached, or replace it with try/catch.
On rejection, reset _isValidating, call markDirty(), and return before
submitting unvalidated values.
🪄 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: 8f2a8778-dfbe-46f8-9b3f-4f9575291fea
📒 Files selected for processing (4)
examples/pomodoro-timer/src/index.tsxexamples/todo-app/src/index.tspackages/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
Move Number.EPSILON before percentage scaling in both renderers.
Adding epsilon after multiplication by 100 can leave the floating-point error unchanged. Apply the fix at the normalized-value scale.
examples/pomodoro-timer/src/index.tsx#L185-L185: useMath.round((this._value + Number.EPSILON) * 100).examples/todo-app/src/index.ts#L107-L107: useMath.round((value + Number.EPSILON) * 100).
📍 Affects 2 files
examples/pomodoro-timer/src/index.tsx#L185-L185(this comment)examples/todo-app/src/index.ts#L107-L107
🤖 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, Move Number.EPSILON
before percentage scaling in both renderers: update the percentage calculation
in examples/pomodoro-timer/src/index.tsx at lines 185-185 and the corresponding
calculation in examples/todo-app/src/index.ts at lines 107-107 to round (value +
Number.EPSILON) after adding epsilon to the normalized value, 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.
At Line 142, .catch(...) is a standalone expression. It is not attached to the Promise.all call at Line 83. This causes the parse error reported by Biome and blocks compilation.
Attach the handler to the Promise.all expression or use try/catch. On rejection, reset _isValidating, call markDirty(), and return before submitting unvalidated values.
Proposed fix
- const results = await Promise.all(validationPromises);
+ const results = await Promise.all(validationPromises).catch((err) => {
+ console.error("Promise.all failed:", err);
+ this._isValidating = false;
+ this.markDirty();
+ return undefined;
+ });
+ if (results === undefined) return;
...
-.catch(err => console.error("Promise.all failed:", err));📝 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.
| .catch(err => console.error("Promise.all failed:", err)); | |
| const results = await Promise.all(validationPromises).catch((err) => { | |
| console.error("Promise.all failed:", err); | |
| this._isValidating = false; | |
| this.markDirty(); | |
| return undefined; | |
| }); | |
| if (results === undefined) return; |
🧰 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: esbuild failed to parse TypeScript/JS: Unexpected "." at src/Form.ts:142:0. Code shown: .catch(err => console.error("Promise.all failed:", err));
[error] 142-142: TypeScript DTS build failed with syntax errors at src/Form.ts(142,1): TS1128 Declaration or statement expected.
[error] 142-142: TypeScript DTS build failed at src/Form.ts(142,2): TS1005 'try' expected.
[error] 142-142: TypeScript DTS build failed at src/Form.ts(142,12): TS1005 ')' expected.
[error] 142-142: TypeScript DTS build failed at src/Form.ts(142,56): TS1005 ';' expected.
[error] 142-142: TypeScript DTS build failed at src/Form.ts(142,52): TS2304 Cannot find name 'err'.
🪛 GitHub Actions: CI / build-and-test
[error] 142-142: esbuild build failed in package @termuijs/ui with error: Unexpected "." at src/Form.ts:142:0. Offending code shown: .catch(err => console.error("Promise.all failed:", err));
[error] 142-142: TypeScript DTS build failed with TS1128: Declaration or statement expected (src/Form.ts:142:1).
[error] 142-142: TypeScript DTS build failed with TS1005: 'try' expected (src/Form.ts:142:2).
[error] 142-142: TypeScript DTS build failed with TS1005: ')' expected (src/Form.ts:142:12).
[error] 142-142: TypeScript DTS build failed with TS1005: ';' expected (src/Form.ts:142:56).
[error] 142-142: TypeScript DTS build failed with TS2304: Cannot find name 'err' (src/Form.ts:142:52).
🤖 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 the Promise.all call so its rejection handler is syntactically attached,
or replace it with try/catch. On rejection, reset _isValidating, call
markDirty(), and return before submitting unvalidated values.
Source: Linters/SAST tools
1 participant
Description
This PR fixes real bugs found in the codebase:
.sort()coerces elements to strings, so[10, 9, 2]sorts as[10, 2, 9]; numeric comparator sorts correctly.Number.EPSILONtoMath.round: prevents floating-point drift (e.g.1.005 * 100rounding to 100 instead of 101).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: #3560
Summary by CodeRabbit