fix: resolve 4 bugs in termui - #3518
Conversation
github-actions
Bot
added
type:bug
📝 WalkthroughWalkthroughThe changes add explicit decimal parsing for tab keys, log rejected form promises, and apply numeric sorting to selection values in ChangesInput parsing
Promise rejection logging
Selection ordering
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 await
Promise.all(validationPromises) by removing the standalone .catch expression and
wrapping the await in try/catch. In the rejection handler, reset _isValidating
and call markDirty(), then return or rethrow without processing results;
preserve normal result handling only after Promise.all resolves.
In `@packages/ui/src/TreeSelect.ts`:
- Around line 185-186: Update the _valuesEqual comparator so its two value
arrays use a type-consistent comparison: since a and b are string values, sort
both with the same lexicographic comparator and remove numeric subtraction.
Preserve order-independent equality and ensure the implementation passes the
packages/ui strict TypeScript check.
🪄 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: 4c484000-c9be-465b-8507-0b37af37d59d
📒 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 correctly.
Line 142 starts a standalone .catch expression. Biome reports this as a parse error, so packages/ui/src/Form.ts cannot compile.
Use try/catch around the await Promise.all(validationPromises) at Line 83. In the rejection path, reset _isValidating and call markDirty() before returning or rethrowing. Do not continue with results after Promise.all rejects.
🧰 Tools
🪛 Biome (2.5.6)
[error] 142-142: Expected a statement but instead found '.catch(err => console.error("Promise.all failed:", err))'.
(parse)
🤖 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 await Promise.all(validationPromises) by removing the standalone .catch
expression and wrapping the await in try/catch. In the rejection handler, reset
_isValidating and call markDirty(), then return or rethrow without processing
results; preserve normal result handling only after Promise.all resolves.
Source: Linters/SAST tools
| const sortedA = [...a].sort((a, b) => a - b); | ||
| const sortedB = [...b].sort(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
npx --no-install tsc --noEmit --pretty falseRepository: Karanjot786/TermUI
Length of output: 5708
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)TreeSelect\.(ts|test\.ts)$|(^|/)tsconfig.*\.json$|package\.json$' || true
echo "== TreeSelect outline =="
ast-grep outline packages/ui/src/TreeSelect.ts || true
echo "== relevant TreeSelect code =="
cat -n packages/ui/src/TreeSelect.ts | sed -n '140,210p'
echo "== test relevant code =="
cat -n packages/ui/src/TreeSelect.test.ts | sed -n '1,120p'
echo "== package tsconfig =="
for f in tsconfig.json packages/tsconfig.json; do
[ -f "$f" ] && { echo "-- $f"; cat -n "$f"; }
done
echo "== build typecheck with project config =="
npx --no-install tsc --showConfig --build --dry 2>&1 | head -50 || true
npx --no-install tsc -b --noEmit 2>&1 | head -200 || trueRepository: Karanjot786/TermUI
Length of output: 11804
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== root/package configs =="
for f in package.json packages/ui/package.json packages/ui/tsconfig.json packages/ui/tsconfig.typecheck.json tsconfig.base.json; do
[ -f "$f" ] && { echo "-- $f"; cat -n "$f"; }
done
echo "== TypeScript invocation with project =="
npx --no-install tsc -p packages/ui/tsconfig.typecheck.json --noEmit 2>&1
echo "== local JS sort/comparator behavior =="
node2 - <<'JS'
for (const pairs of [
[['2', '10'], ['10', '2']],
[['s', 'a'], ['a', 's']],
[['one', 'two'], ['two', 'one']],
]) {
const a = [...pairs[0]].sort((a, b) => a - b);
const b = [...pairs[0]].sort();
console.log(pairs[0].join(','), 'cmpA=', a.join(','), 'sort=', b.join(','));
}
JSRepository: Karanjot786/TermUI
Length of output: 5927
Fix the value-equality comparator.
a and b are string values in _valuesEqual, so subtracting them fails the packages/ui TypeScript strict check. If the values are meant to be strings, use the same lexicographic sort for both arrays; if numeric values are required, make the value type/comparator consistent and add tests for numeric and nonnumeric inputs.
🤖 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` around lines 185 - 186, Update the
_valuesEqual comparator so its two value arrays use a type-consistent
comparison: since a and b are string values, sort both with the same
lexicographic comparator and remove numeric subtraction. Preserve
order-independent equality and ensure the implementation passes the packages/ui
strict TypeScript check.
Source: Coding guidelines
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: #3517
Summary by CodeRabbit