fix: resolve 4 bugs in termui - #3470
Conversation
github-actions
Bot
added
type:bug
📝 WalkthroughWalkthroughChangesRSS hexadecimal parsing
Weather polling lifecycle
Form promise error handling
MultiSelect ordering
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.5)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
🧹 Nitpick comments (1)
packages/ui/src/MultiSelect.ts (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for multi-digit indexes.
The numeric comparator is correct, but
MultiSelect.test.ts:104-114only covers indexes0through2. A lexicographic sort would also pass that test. Add a case with at least 11 options, select index10before index9, and assert thatselectedOptionsreturns option9before option10.🤖 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/MultiSelect.ts` at line 33, Add a regression case in MultiSelect.test.ts covering at least 11 options: select index 10 before index 9, then assert selectedOptions returns option 9 followed by option 10. Keep the existing tests unchanged and target the selectedOptions behavior of MultiSelect.
🤖 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/rss-reader/src/index.tsx`:
- Around line 30-31: Update the entity decoding logic around codePoint to parse
the full captured hexadecimal digit string, reject entities with more than eight
digits, and reject values above 0x10FFFF before calling String.fromCodePoint;
preserve returning match for invalid entities.
In `@packages/ui/src/Form.ts`:
- Around line 141-142: Remove the stray catch after the Form class closes so
Form.ts compiles. If rejection handling is required, attach it to the
Promise.all(validationPromises) flow inside submit(), reset _isValidating, call
markDirty(), and then return.
---
Nitpick comments:
In `@packages/ui/src/MultiSelect.ts`:
- Line 33: Add a regression case in MultiSelect.test.ts covering at least 11
options: select index 10 before index 9, then assert selectedOptions returns
option 9 followed by option 10. Keep the existing tests unchanged and target the
selectedOptions behavior of MultiSelect.
🪄 Autofix (Beta)
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: 6c9fe756-9071-4c39-99b1-d5caac173e5e
📒 Files selected for processing (4)
examples/rss-reader/src/index.tsxexamples/weather/src/index.tsxpackages/ui/src/Form.tspackages/ui/src/MultiSelect.ts
| const codePoint = Number.parseInt(entity.slice(2, 10), 16); | ||
| return Number.isFinite(codePoint) ? String.fromCodePoint(codePoint) : match; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
node - <<'EOF'
for (const entity of ['`#x00000041FF`', '`#xFFFFFFFF`']) {
const codePoint = Number.parseInt(entity.slice(2, 10), 16);
try {
console.log(entity, codePoint, String.fromCodePoint(codePoint));
} catch (error) {
console.log(entity, codePoint, error.name);
}
}
EOFRepository: Karanjot786/TermUI
Length of output: 207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "File location and relevant lines:"
if [ -f examples/rss-reader/src/index.tsx ]; then
nl -ba examples/rss-reader/src/index.tsx | sed -n '1,80p'
else
echo "examples/rss-reader/src/index.tsx not found"
fi
echo
echo "Search for decode helpers and usages:"
rg -n "decode|fromCodePoint|parseInt\\(|&`#x`|fromCharCode|value" examples/rss-reader/src packages -S 2>/dev/null | head -200 || true
echo
echo "Check for error boundaries/error handling in example and app:"
rg -n "ErrorBoundary|try|catch|fallback|error" examples/rss-reader/src -S 2>/dev/null || trueRepository: Karanjot786/TermUI
Length of output: 230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "File location and relevant lines:"
if [ -f examples/rss-reader/src/index.tsx ]; then
awk '{printf "%6d\t%s\n", NR, $0}' examples/rss-reader/src/index.tsx | sed -n '1,80p'
else
echo "examples/rss-reader/src/index.tsx not found"
fi
echo
echo "Search for decode helpers and usages:"
rg -n "decode|fromCodePoint|parseInt\\(|&`#x`|fromCharCode|value" examples/rss-reader/src packages -S 2>/dev/null | head -200 || true
echo
echo "Check for error boundaries/error handling in example and app:"
rg -n "ErrorBoundary|try|catch|fallback|error" examples/rss-reader/src -S 2>/dev/null || trueRepository: Karanjot786/TermUI
Length of output: 25847
Validate the complete entity before conversion.
entity.slice(2, 10) silently discards hexadecimal digits after the eighth. For example, 䇿 is decoded as A. Also, � produces a finite number outside the valid Unicode range, so String.fromCodePoint throws RangeError.
Use the full captured digit string for parseInt, reject sequences longer than eight digits, and reject values greater than 0x10FFFF before calling String.fromCodePoint.
🤖 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/rss-reader/src/index.tsx` around lines 30 - 31, Update the entity
decoding logic around codePoint to parse the full captured hexadecimal digit
string, reject entities with more than eight digits, and reject values above
0x10FFFF before calling String.fromCodePoint; preserve returning match for
invalid entities.
|
|
||
| .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
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '78,146p' packages/ui/src/Form.ts
rg -n -C 3 'Promise\.all|\.catch|_isValidating|markDirty' packages/ui/src/Form.tsRepository: Karanjot786/TermUI
Length of output: 5634
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,146p' packages/ui/src/Form.ts
python3 - <<'PY'
from pathlib import Path
text = Path("packages/ui/src/Form.ts").read_text()
try:
import black
except Exception as e:
print(f"black unavailable: {type(e).__name__}: {e}")
print("Unable to run black parser check.")
else:
try:
black.format_str(text, mode=black.Mode())
print("black parse check: ok")
except Exception as e:
print("black parse check: failed")
print(e)
PYRepository: Karanjot786/TermUI
Length of output: 6748
Remove the stray .catch after Form closes.
The final }\n.catch(...) outside the class body is invalid TypeScript, so packages/ui/src/Form.ts cannot compile. If rejection handling is needed, attach it to the Promise.all(validationPromises) call inside submit(), then reset _isValidating and call markDirty() before returning.
🧰 Tools
🪛 Biome (2.5.5)
[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: tsup/esbuild build failed: Unexpected "." at .catch(err => console.error("Promise.all failed:", err));. The @termuijs/ui build command exited with code 1.
🪛 GitHub Actions: CI / build-and-test
[error] 142-142: The @termuijs/ui build failed during tsup/esbuild compilation بسبب an unexpected '.' in .catch(err => console.error("Promise.all failed:", 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, Remove the stray catch after
the Form class closes so Form.ts compiles. If rejection handling is required,
attach it to the Promise.all(validationPromises) flow inside submit(), reset
_isValidating, call markDirty(), and then return.
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.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: #3469
Summary by CodeRabbit