fix: resolve 4 bugs in termui - #3502
Conversation
github-actions
Bot
added
area:examples
📝 WalkthroughWalkthroughThe changes update four independent code paths: a form keyboard condition, RSS hexadecimal entity parsing, form promise rejection logging, and multi-select option ordering. ChangesForm validation example
RSS entity parsing
Form rejection logging
Multi-select 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.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: 3
🧹 Nitpick comments (1)
packages/ui/src/MultiSelect.ts (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for multi-digit indexes.
Line [33] correctly applies numeric sorting. The existing test at
packages/ui/src/MultiSelect.test.ts:126-133selects indexes 0 and 2, so the old lexicographic sort also passes. Add a case with at least 11 options and selected indexes 2 and 10.🤖 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 the MultiSelect tests covering at least 11 options with indexes 2 and 10 selected, and assert the returned options remain in numeric index order. Keep the existing coverage unchanged and target the behavior exposed by MultiSelect’s checked-options sorting.
🤖 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/forms-and-validation/src/index.tsx`:
- Line 125: In the keyboard handler condition, replace the invalid `event.ctrl
!` expression with the valid negated boolean check `!event.ctrl`, preserving the
existing Ctrl+C quit behavior and handling plain `c` as intended.
In `@examples/rss-reader/src/index.tsx`:
- Around line 30-31: Update the entity-decoding logic around codePoint to parse
the full hexadecimal suffix rather than truncating it, return match when the
suffix exceeds eight hexadecimal digits, and only call String.fromCodePoint for
finite values at or below 0x10FFFF; otherwise preserve the original match.
In `@packages/ui/src/Form.ts`:
- Around line 141-142: Move the trailing rejection handler into the Form class’s
submit() method by wrapping await Promise.all(validationPromises) in
try/catch/finally; keep _isValidating = false and markDirty() in finally so
rejected validators reset validation state, and remove the out-of-class .catch
that breaks parsing.
---
Nitpick comments:
In `@packages/ui/src/MultiSelect.ts`:
- Line 33: Add a regression case in the MultiSelect tests covering at least 11
options with indexes 2 and 10 selected, and assert the returned options remain
in numeric index order. Keep the existing coverage unchanged and target the
behavior exposed by MultiSelect’s checked-options sorting.
🪄 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: 396372fe-e3f7-4e2c-9ad4-622f9e1ea257
📒 Files selected for processing (4)
examples/forms-and-validation/src/index.tsxexamples/rss-reader/src/index.tsxpackages/ui/src/Form.tspackages/ui/src/MultiSelect.ts
| } | ||
|
|
||
| if (event.key === 'c' && event.ctrl === false) { | ||
| if (event.key === 'c' && event.ctrl !) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Replace the invalid condition with a valid boolean check.
event.ctrl ! is invalid TypeScript syntax. The example will fail to parse and cannot build or run. Use !event.ctrl to handle plain c without changing the existing Ctrl+C quit behavior.
Proposed fix
- if (event.key === 'c' && event.ctrl !) {
+ if (event.key === 'c' && !event.ctrl) {📝 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.
| if (event.key === 'c' && event.ctrl !) { | |
| if (event.key === 'c' && !event.ctrl) { |
🤖 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/forms-and-validation/src/index.tsx` at line 125, In the keyboard
handler condition, replace the invalid `event.ctrl !` expression with the valid
negated boolean check `!event.ctrl`, preserving the existing Ctrl+C quit
behavior and handling plain `c` as intended.
| 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:
#!/bin/bash
set -euo pipefail
node -e '
try {
String.fromCodePoint(0x110000);
process.exit(1);
} catch (error) {
if (!(error instanceof RangeError)) process.exit(2);
}
'
rg -n -C 4 'decodeEntities|String\.fromCodePoint|&`#x`[0-9A-Fa-f]' \
examples/rss-reader/src/index.tsxRepository: Karanjot786/TermUI
Length of output: 1658
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const tests = [
['&`#x110000`;', false],
['&`#x123456789`;', false],
];
for (const [input, shouldThrow] of tests) {
const entity = input.slice(1, -1);
const slice = entity.slice(2, 10);
const codePoint = Number.parseInt(slice, 16);
let threw = false;
let value;
try {
value = String.fromCodePoint(codePoint);
} catch (error) {
threw = true;
}
console.log(JSON.stringify({
input,
sliced: slice,
codePoint: codePoint.toString(16),
decoded: value,
threw,
finite: Number.isFinite(codePoint)
}));
}
JSRepository: Karanjot786/TermUI
Length of output: 341
Reject oversized hexadecimal code points before decoding.
entity.slice(2, 10) truncates longer hexadecimal entities, so � decodes � and loses the final digit. The same path also lets values above 0x10FFFF, such as �, pass Number.isFinite and throw RangeError from String.fromCodePoint. Use the full hexadecimal suffix, return match when it is longer than eight digits, and reject values greater than 0x10FFFF.
Proposed fix
- const codePoint = Number.parseInt(entity.slice(2, 10), 16);
- return Number.isFinite(codePoint) ? String.fromCodePoint(codePoint) : match;
+ const hex = entity.slice(2);
+ if (hex.length > 8) return match;
+ const codePoint = Number.parseInt(hex, 16);
+ return codePoint <= 0x10FFFF ? String.fromCodePoint(codePoint) : match;📝 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.
| const codePoint = Number.parseInt(entity.slice(2, 10), 16); | |
| return Number.isFinite(codePoint) ? String.fromCodePoint(codePoint) : match; | |
| const hex = entity.slice(2); | |
| if (hex.length > 8) return match; | |
| const codePoint = Number.parseInt(hex, 16); | |
| return codePoint <= 0x10FFFF ? String.fromCodePoint(codePoint) : match; |
🤖 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 hexadecimal suffix
rather than truncating it, return match when the suffix exceeds eight
hexadecimal digits, and only call String.fromCodePoint for finite values at or
below 0x10FFFF; otherwise preserve the original match.
|
|
||
| .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
npx biome check packages/ui/src/Form.tsRepository: Karanjot786/TermUI
Length of output: 2385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Form.ts outline =="
ast-grep outline packages/ui/src/Form.ts || true
echo "== Relevant lines around submit/catch =="
nl -ba packages/ui/src/Form.ts | sed -n '70,105p;132,148p'
echo "== parse/typecheck with local tsc if available =="
if command -v tsc >/dev/null 2>&1; then
tsc --version
tsc --noEmit packages/ui/src/Form.ts 2>&1 || true
else
echo "tsc not available"
fi
echo "== package scripts and devDependency config =="
sed -n '1,220p' package.jsonRepository: Karanjot786/TermUI
Length of output: 863
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Relevant lines around submit/catch =="
awk 'NR>=70 && NR<=105 {printf "%6d\t%s\n", NR, $0} NR>=132 && NR<=148 {printf "%6d\t%s\n", NR, $0}' packages/ui/src/Form.ts
echo "== package scripts and devDependency config =="
awk '{printf "%6d\t%s\n", NR, $0}' package.json | sed -n '1,220p'
echo "== TypeScript/biome availability and parse probe =="
for cmd in npm npx tsc biome pnpm yarn; do
command -v "$cmd" >/dev/null 2>&1 && printf '%s=%s\n' "$cmd" "$(command -v "$cmd")"
done
if command -v node >/dev/null 2>&1; then
node - <<'JS'
const fs = require('fs');
const src = fs.readFileSync('packages/ui/src/Form.ts', 'utf8');
console.log(JSON.stringify({
submitOpenMethods: src.includes('async submit('),
catchFoundOutsideMethodLikePattern: /\.catch\(err => console\.error\("Promise\.all failed:", err\)\)/.test(src),
fileLength: src.length
}, null, 2));
try {
require('typescript').createSourceFile('Form.ts', src, require('typescript').ScriptTarget.Latest, true);
console.log({typescript_parse: "ok"});
} catch (e) {
console.log({typescript_parse_error: String(e.message)});
}
try {
new Function(src);
console.log({js_parse: "ok"});
} catch (e) {
console.log({js_parse_error: String(e.message)});
}
JS
fiRepository: Karanjot786/TermUI
Length of output: 4231
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full package/tsconfig references =="
git ls-files | rg '(^|/)(tsconfig.*json|package.json)$|packages/ui/(package.json|tsconfig.*json)$'
for f in package.json packages/ui/package.json $(git ls-files | rg '(^|/)tsconfig.*json$'); do
echo "--- $f"
sed -n '1,220p' "$f"
done
echo "== Parse check with installed TypeScript =="
node - <<'JS'
const path = require('node:path');
const tsPath = path.join(process.cwd(), 'node_modules', 'typescript');
const ts = require(tsPath);
const src = require('node:fs').readFileSync('packages/ui/src/Form.ts', 'utf8');
const fileName = 'packages/ui/src/Form.ts';
const sourceFile = ts.createSourceFile(fileName, src, ts.ScriptTarget.Latest, true);
sourceFile.forEachChild(node => {
const text = (n) => ts.SyntaxKind[n.kind];
});
function walk(node, indent = 0) {
console.log(`${' '.repeat(indent)}${ts.SyntaxKind[node.kind]} ${node.getStart()}-${node.getEnd()}`);
ts.forEachChild(node, n => walk(n, indent + 2));
}
walk(sourceFile);
const diagnostics = ts.getPreEmitDiagnostics(sourceFile, fs ? fs : undefined);
for (const diag of diagnostics) {
const message = ts.flattenDiagnosticMessageText(diag.messageText, '\n');
const loc = diag.file
? `${diag.file.fileName} ${diag.file.getLineAndCharacterOfPosition(diag.start || 0).line + 1}`
: 'global';
console.log(`${loc}: ${diag.code}: ${message}`);
}
try {
new Function(src);
console.log({js_parse: "ok"});
} catch (e) {
console.log({js_parse_error: String(e.message)});
}
JSRepository: Karanjot786/TermUI
Length of output: 23040
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File length and trailing catch location =="
wc -l packages/ui/src/Form.ts
awk 'NR>=96 && NR<=102 || NR>=139 && NR<=144 {printf "%d: %s\n", NR, $0}' packages/ui/src/Form.ts
echo "== Installed TypeScript compiler location and parse run =="
node - <<'JS'
const { exec } = require('node:child_process');
const src = require('node:fs').readFileSync('packages/ui/src/Form.ts', 'utf8');
const file = require('node:fs').writeFileSync('/tmp/Form-outer-catch.ts', src, 'utf8');
exec('node ./node_modules/typescript/lib/tsc.js --noEmit --target ES2022 --module ESNext /tmp/Form-outer-catch.ts', (err, stdout, stderr) => {
console.log('typescript_path=', require.resolve('typescript', { paths: [process.cwd()] }));
console.log('exit=', err ? err.code : 0);
console.log('stdout=', stdout);
console.log('stderr=', stderr);
});
JSRepository: Karanjot786/TermUI
Length of output: 1423
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Run installed global tsc =="
tsc --version
tsc --noEmit --target ES2022 --module ESNext packages/ui/src/Form.ts 2>&1
echo "== Run installed global TypeScript parser =="
node "$TSC_NODE_OPTIONS" "$(which node)" -e '
const path = require("node:path");
const ts = require(path.dirname(process.argv[2]) + "/typescript/lib/typescript.js");
const src = require("node:fs").readFileSync("packages/ui/src/Form.ts", "utf8");
const sourceFile = ts.createSourceFile("Form.ts", src, ts.ScriptTarget.Latest, true);
const diagnostics = ts.getPreEmitDiagnostics(sourceFile);
for (const diag of diagnostics) {
const pos = diag.file && diag.start !== undefined ? diag.file.getLineAndCharacterOfPosition(diag.start) : null;
console.log((diag.file || global) + ":" + (pos && pos.line + 1 || "") + ":" + (pos && pos.character + 1 || "") + " TS" + diag.code + ": " + diag.messageText);
}
'Repository: Karanjot786/TermUI
Length of output: 466
🏁 Script executed:
#!/bin/bash
set -euo pipefailRepository: Karanjot786/TermUI
Length of output: 156
Move the rejection handler inside submit().
Line 142 appends .catch(...) after the Form class ends, which makes packages/ui/src/Form.ts fail to parse and fail npm run typecheck. Wrap the await Promise.all(validationPromises) call in try/catch/finally, and keep _isValidating = false and markDirty() in finally so a rejected validator still resets validation state.
🧰 Tools
🪛 Biome (2.5.5)
[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, Move the trailing rejection
handler into the Form class’s submit() method by wrapping await
Promise.all(validationPromises) in try/catch/finally; keep _isValidating = false
and markDirty() in finally so rejected validators reset validation state, and
remove the out-of-class .catch that breaks parsing.
Source: Linters/SAST tools
1 participant
Description
This PR fixes real bugs found in the codebase:
x === trueis equivalent tox(andx === falseto!x), and shorter to read.Promise.all: an unhandled rejection in any input promise previously crashed silently.parseInt: without10, strings like'0x1F'or'08'parse in unintended bases..sort()coerces elements to strings, so[10, 9, 2]sorts as[10, 2, 9]; numeric comparator sorts correctly.Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3501
Summary by CodeRabbit