fix: resolve 4 bugs in termui - #3573
Conversation
github-actions
Bot
added
area:examples
📝 WalkthroughWalkthroughThe changes make numeric parsing and sorting explicit in widget and selection logic. Form promise aggregation now logs rejected operations. ChangesNumeric input and ordering corrections
Promise failure logging
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: 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 `@packages/ui/src/Form.ts`:
- Around line 141-142: Fix the validation flow around Promise.all so its
rejection is handled with a surrounding try/catch rather than a standalone
.catch statement. On rejection, log the error, set _isValidating to false, call
markDirty(), and return or rethrow; ensure execution does not continue iterating
over results after aggregation fails.
In `@packages/ui/src/TreeSelect.ts`:
- Line 185: Update the sorting logic in TreeSelect to use a shared numeric
comparator for both string arrays, parsing each operand as a base-10 integer
before comparison. Apply the same comparator to sortedA and sortedB so
decimal-string values compare consistently regardless of insertion order, while
keeping TypeScript strict-mode compilation valid.
🪄 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: c75a4577-4358-400f-8830-93acdf61724f
📒 Files selected for processing (4)
examples/widget-gallery/src/index.tspackages/ui/src/Form.tspackages/ui/src/MultiSelect.tspackages/ui/src/TreeSelect.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.
At Line 142, .catch(...) starts a standalone statement. This causes the parse error reported by Biome and prevents the package from building.
Wrap the Promise.all call at Line 83 in try/catch. When validation rejects, log the error, set _isValidating to false, call markDirty(), and return or rethrow. Do not continue to iterate over results after the aggregation fails.
🧰 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: 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 including missing try, closing parenthesis, and semicolon. Command failed with exit code 1.
🪛 GitHub Actions: CI / build-and-test
[error] 142-142: Build failed during the tsup/esbuild build. Unexpected '.' at the standalone '.catch(err => console.error("Promise.all failed:", err));'. TypeScript also reports declaration/statement, ')' and ';' syntax errors, and cannot find name 'err'.
🤖 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 its rejection is handled with a surrounding try/catch
rather than a standalone .catch statement. On rejection, log the error, set
_isValidating to false, call markDirty(), and return or rethrow; ensure
execution does not continue iterating over results after aggregation fails.
Source: Linters/SAST tools
| function _valuesEqual(a: string[], b: string[]): boolean { | ||
| if (a.length !== b.length) return false; | ||
| const sortedA = [...a].sort(); | ||
| const sortedA = [...a].sort((a, b) => a - b); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Use one numeric comparator for both string arrays.
a and b are strings, so a - b causes a TypeScript compile error. sortedB also remains lexicographic, which makes equal numeric values compare unequal in different insertion orders.
If tree values are decimal strings, parse both operands with radix 10 and reuse the comparator:
Proposed fix
function _valuesEqual(a: string[], b: string[]): boolean {
if (a.length !== b.length) return false;
- const sortedA = [...a].sort((a, b) => a - b);
- const sortedB = [...b].sort();
+ const compareNumeric = (left: string, right: string) =>
+ parseInt(left, 10) - parseInt(right, 10);
+ const sortedA = [...a].sort(compareNumeric);
+ const sortedB = [...b].sort(compareNumeric);As per coding guidelines, TypeScript files must use strict mode.
#!/bin/bash
set -euo pipefail
config="$(fd -a -t f 'tsconfig.*\.json$' | head -n 1)"
test -n "$config"
npx tsc --noEmit --pretty false -p "$config"🤖 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/TreeSelect.ts` at line 185, Update the sorting logic in
TreeSelect to use a shared numeric comparator for both string arrays, parsing
each operand as a base-10 integer before comparison. Apply the same comparator
to sortedA and sortedB so decimal-string values compare consistently regardless
of insertion order, while keeping TypeScript strict-mode compilation valid.
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.Promise.all: an unhandled rejection in any input promise previously crashed silently..sort()coerces elements to strings, so[10, 9, 2]sorts as[10, 2, 9]; numeric comparator sorts correctly.parseInt: without10, strings like'0x1F'or'08'parse in unintended bases.Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3572
Summary by CodeRabbit