fix: resolve 4 bugs in termui - #3519
Conversation
github-actions
Bot
added
area:examples
📝 WalkthroughWalkthroughThe changes apply three independent correctness fixes: epsilon-adjusted percentage rounding, strict numeric validation for prompt choices, and explicit dependency-name sorting. ChangesPercentage rounding
Choice validation
Dependency sorting
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🧪 Generate unit tests (beta)
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
🤖 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/todo-app/src/index.ts`:
- Line 107: Update the percentage calculation using pct so the epsilon is
applied before scaling, or equivalently scale the epsilon for percentage units,
ensuring boundary value 0.145 rounds to 15. Add a boundary test covering value =
0.145 and preserve the existing rounding behavior for other values.
In `@packages/ui/src/prompts.ts`:
- Around line 115-116: Update the choice parser around n to validate the entire
trimmed token rather than relying on parseInt, rejecting malformed suffixes such
as 1abc and fractional values such as 1.5 while accepting only valid integer
choices within choices.length. Add regression coverage in the prompts tests for
both invalid input cases.
In `@scripts/build-registry.ts`:
- Line 47: Update the dependency sorting in collectDeps to use a lexicographic
string comparator rather than numeric subtraction, preserving alphabetical
ordering of the Set<string> values.
🪄 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: 44c173dc-ea75-4b9f-9b86-ecc3ce95bd8b
📒 Files selected for processing (3)
examples/todo-app/src/index.tspackages/ui/src/prompts.tsscripts/build-registry.ts
| const filled = Math.round(barWidth * value); | ||
|
|
||
| const pct = Math.round(value * 100); | ||
| const pct = Math.round(value * 100 + Number.EPSILON); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Apply the epsilon before scaling.
Number.EPSILON is too small after value * 100 for some boundary values. For example, 0.145 * 100 becomes 14.499999999999998; adding Number.EPSILON still rounds to 14 instead of 15.
Move the epsilon before multiplication, or scale it to the percentage value. Add a boundary test for value = 0.145.
Proposed fix
- const pct = Math.round(value * 100 + Number.EPSILON);
+ const pct = Math.round((value + Number.EPSILON) * 100);📝 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 pct = Math.round(value * 100 + Number.EPSILON); | |
| const pct = Math.round((value + Number.EPSILON) * 100); |
🤖 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/todo-app/src/index.ts` at line 107, Update the percentage
calculation using pct so the epsilon is applied before scaling, or equivalently
scale the epsilon for percentage units, ensuring boundary value 0.145 rounds to
15. Add a boundary test covering value = 0.145 and preserve the existing
rounding behavior for other values.
| const n = parseInt(trimmed, 10); | ||
| if (!isNaN(n) && n >= 1 && n <= choices.length) { | ||
| if (!Number.isNaN(n) && n >= 1 && n <= choices.length) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)prompts\.(ts|test\.ts)$|(^|/)prompts\.ts$' || true
echo "== prompts outline =="
if [ -f packages/ui/src/prompts.ts ]; then
ast-grep outline packages/ui/src/prompts.ts || true
echo "== relevant lines =="
nl -ba packages/ui/src/prompts.ts | sed -n '90,135p'
fi
echo "== tests around prompts =="
if [ -f packages/ui/src/prompts.test.ts ]; then
nl -ba packages/ui/src/prompts.test.ts | sed -n '1,220p'
fi
echo "== behavioral probe for JS parseInt Number Number.isInteger =="
node - <<'JS'
const vals = ["1", " 1 ", "1abc", "1.5", "-01", "abc", "1e2", "01abc", "1.abc", ""];
for (const trimmed of vals.map(s => s.trim())) {
const parseIntVal = parseInt(trimmed, 10);
const numberVal = Number(trimmed);
console.log(JSON.stringify({
input: trimmed,
parseInt: parseIntVal,
Number: numberVal,
Number_isInteger: Number.isInteger(numberVal)
}));
}
JSRepository: Karanjot786/TermUI
Length of output: 1799
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant lines: packages/ui/src/prompts.ts =="
awk 'NR>=91 && NR<=130 {printf "%5d\t%s\n", NR, $0}' packages/ui/src/prompts.ts
echo "== prompts tests first 240 lines =="
if [ -f packages/ui/src/prompts.test.ts ]; then
awk '{printf "%5d\t%s\n", NR, $0}' packages/ui/src/prompts.test.ts | sed -n '1,240p'
fi
echo "== behavioral probe for JS parseInt Number Number.isInteger =="
node - <<'JS'
const vals = ["1", " 1 ", "1abc", "1.5", "-01", "abc", "1e2", "01abc", "1.abc", ""];
for (const trimmed of vals.map(s => s.trim())) {
const parseIntVal = parseInt(trimmed, 10);
const numberVal = Number(trimmed);
console.log(JSON.stringify({input: trimmed, parseInt: parseIntVal, Number: numberVal, Number_isInteger: Number.isInteger(numberVal)}));
}
JSRepository: Karanjot786/TermUI
Length of output: 5429
Make the choice parser validate the full input.
n comes from parseInt, so it is always a number or NaN; Number.isNaN(n) and isNaN(n) produce the same result. The parser also accepts inputs like 1abc because parseInt returns 1. Parse the complete token and require an integer. Add regression tests for malformed inputs such as 1abc and fractional inputs such as 1.5 in packages/ui/src/prompts.test ts.
🤖 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/prompts.ts` around lines 115 - 116, Update the choice parser
around n to validate the entire trimmed token rather than relying on parseInt,
rejecting malformed suffixes such as 1abc and fractional values such as 1.5
while accepting only valid integer choices within choices.length. Add regression
coverage in the prompts tests for both invalid input cases.
| let m: RegExpExecArray | null; | ||
| while ((m = re.exec(content)) !== null) deps.add(m[1]!); | ||
| return [...deps].sort(); | ||
| return [...deps].sort((a, b) => a - b); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'const deps = new Set<string>|sort\(\(a, b\) => a - b\)' scripts/build-registry.ts
rg -n -C 8 'collects unique sorted' scripts/build-registry.test.tsRepository: Karanjot786/TermUI
Length of output: 1266
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'TypeScript compiler availability:\n'
(tsc --version 2>&1) || true
printf '\nbuild-registry relevant lines:\n'
sed -n '30,55p' scripts/build-registry.ts | cat -n
printf '\nDependency parsing context:\n'
rg -n -C 2 'collectDeps|toSlug|buildRegistry|registry' scripts/build-registry.ts
printf '\nTypeScript type-check probe for subtraction of strings:\n'
tmp="$(mktemp -d)"
cat > "$tmp/minus.ts" <<'TS'
const deps = new Set<string>();
const sorted = [...deps].sort((a: string, b: string) => a - b);
export { sorted };
TS
if command -v tsc >/dev/null 2>&1; then
tsc --strict --noEmit "$tmp/minus.ts" 2>&1
else
node - <<'JS'
const deps = new Set(['b', 'a']);
const sorted = [...deps].sort((a, b) => a - b);
console.log('runtime sorted:', JSON.stringify(sorted));
JS
fiRepository: Karanjot786/TermUI
Length of output: 4344
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Runtime sub-sort probe:\n'
node - <<'JS'
const samples = [
['b', 'a'],
['`@termuijs/widgets`', '`@termuijs/core`'],
['z', 'aa'],
];
for (const deps of samples) {
const sortedSub = [...deps].sort((a, b) => a - b);
const sortedCompare = [...deps].sort((a, b) => a.localeCompare(b));
console.log(JSON.stringify({ input: deps, subSorted: sortedSub, localeCompare: sortedCompare }));
}
JS
printf '\nPackage TypeScript declaration check:\n'
sed -n '1,120p' package.json | json_pp 2>/dev/null || sed -n '1,120p' package.jsonRepository: Karanjot786/TermUI
Length of output: 1794
Use a string comparator for dependency sorting.
collectDeps stores Set<string> values, but sorts with a - b. TypeScript rejects subtraction on strings, and string subtraction produces NaN, so the comparator does not change non-alphabetical input.
- return [...deps].sort((a, b) => a - b);
+ return [...deps].sort((a, b) => a.localeCompare(b));📝 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.
| return [...deps].sort((a, b) => a - b); | |
| return [...deps].sort((a, b) => a.localeCompare(b)); |
🤖 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 `@scripts/build-registry.ts` at line 47, Update the dependency sorting in
collectDeps to use a lexicographic string comparator rather than numeric
subtraction, preserving alphabetical ordering of the Set<string> values.
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.x === trueis equivalent tox(andx === falseto!x), and shorter to read.isNaNwithNumber.isNaN: the global version coerces its argument, soisNaN('1')returns false whileNumber.isNaNis strict.Number.EPSILONtoMath.round: prevents floating-point drift (e.g.1.005 * 100rounding to 100 instead of 101).Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3518
Summary by CodeRabbit