fix: resolve 4 bugs in termui - #3512
Conversation
github-actions
Bot
added
type:bug
📝 WalkthroughWalkthroughThe pull request updates calculator token validation, form promise rejection logging, and tree selection array comparison. ChangesCalculator validation
Form error logging
Tree selection comparison
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 the awaited Promise.all
and the trailing catch expression: attach rejection handling directly to the
promise or wrap the await in try/catch. On rejection, reset _isValidating, call
markDirty(), and return so the form cannot remain stuck when validation fails.
In `@packages/ui/src/TreeSelect.ts`:
- Line 185: Update _valuesEqual to use a shared string comparator when sorting
both value arrays, including selectedValues, instead of numeric subtraction.
Ensure arbitrary string values are compared consistently and remain valid under
strict TypeScript.
🪄 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: 0269f72a-9b13-44d4-8b40-d26ce6fb2c3a
📒 Files selected for processing (3)
examples/calculator/src/index.tsxpackages/ui/src/Form.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
Fix the invalid .catch expression before merging.
Promise.all is awaited at Line 83, but .catch starts as a separate statement at Lines 141-142. This is invalid TypeScript, and Biome reports a parse error.
Attach the handler directly to the promise or wrap Line 83 in try/catch. On rejection, reset _isValidating and call markDirty() before returning; otherwise Lines 92 and 97 are skipped and the form can remain stuck in validation.
🧰 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: unexpected '.' at .catch(err => console.error("Promise.all failed:", err));. TypeScript also reported malformed syntax (TS1128, TS1005, and TS2304).
🪛 GitHub Actions: CI / build-and-test
[error] 142-142: @termuijs/ui build failed during tsup/esbuild: unexpected '.' at '.catch(err => console.error("Promise.all failed:", err));'. TypeScript also reports syntax errors (TS1128, TS1005, TS2304).
🤖 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 awaited Promise.all and the trailing catch expression: attach
rejection handling directly to the promise or wrap the await in try/catch. On
rejection, reset _isValidating, call markDirty(), and return so the form cannot
remain stuck when validation 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 | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 '_valuesEqual|sortedA|sortedB|node\.value|selectedValues' \
packages/ui/src/TreeSelect.ts packages/ui/src/TreeSelect.test.ts
fd -H 'tsconfig*.json' . --exec rg -n '"strict"\s*:\s*true' {}Repository: Karanjot786/TermUI
Length of output: 7492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## package.json scripts / dependencies relevant to type checking"
python3 - <<'PY'
import json
from pathlib import Path
for p in [Path('package.json'), Path('packages/ui/package.json')]:
if p.exists():
data=json.loads(p.read_text())
print(f"--- {p} ---")
print(json.dumps({'scripts': data.get('scripts'), 'devDependencies': data.get('devDependencies'), 'dependencies': data.get('dependencies')}, indent=2, sort_keys=True))
PY
echo
echo "## TreeSelect root/root helpers"
sed -n '1,180p' packages/ui/src/TreeSelect.ts | cat -n
echo
echo "## TreeSelect tests around value content"
sed -n '1,130p' packages/ui/src/TreeSelect.test.ts | cat -n
echo
echo "## TypeScript availability and strict parameter subtraction check"
node - <<'JS'
try {
require.resolve('typescript');
console.log('typescript available');
} catch {
console.log('typescript unavailable');
}
JSRepository: Karanjot786/TermUI
Length of output: 12989
Use the same comparator for both value arrays.
_valuesEqual accepts arbitrary string[], and a - b is invalid in strict TypeScript. The same concern appears for the selectedValues contract, which is not limited to numeric strings. Use one string sort comparator for both arrays and handle non-numeric values consistently.
\
🤖 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 _valuesEqual to use a
shared string comparator when sorting both value arrays, including
selectedValues, instead of numeric subtraction. Ensure arbitrary string values
are compared consistently and remain valid under strict TypeScript.
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.trim()to''misses whitespace-only input;.trim().length === 0is explicit.isNaNwithNumber.isNaN: the global version coerces its argument, soisNaN('1')returns false whileNumber.isNaNis strict.Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3511
Summary by CodeRabbit