fix: resolve 4 bugs in termui - #3511
Conversation
📝 WalkthroughWalkthroughThe changes update progress rounding, select prompt validation, and dependency sorting. Each change affects one line in a separate part of the repository. ChangesProgress percentage rounding
Select input 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 assigned to pct so Number.EPSILON
is added to value before multiplying by 100, then retain Math.round for the
final integer percentage.
In `@packages/ui/src/prompts.ts`:
- Line 116: Update the select-input validation around the parsed variable n to
reject malformed values such as “2abc”: validate that the complete user input
represents an integer, then retain the existing 1-to-choices.length range check
before selecting an option.
In `@scripts/build-registry.ts`:
- Line 47: Update the dependency sorting in the registry-building flow to use
lexicographic string ordering rather than numeric subtraction. In the visible
sort expression within the dependency collection logic, use the default string
comparator so package names such as `@termuijs/core`, `@termuijs/motion`, and
`@termuijs/widgets` are emitted deterministically.
🪄 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: df4e2bf6-fb43-4ced-866e-8950865672a3
📒 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
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '80,125p' examples/todo-app/src/index.ts
echo
echo "== occurrences of pct/value =="
rg -n "const pct|value \* 100|Number\.EPSILON|progress|percent" examples/todo-app/src/index.ts
echo
echo "== JS behavior probe =="
node - <<'JS'
const values = [0.285, 0.1, 0.5, 0.165, 0.28499999999999995, 0.28500000000000004];
for (const value of values) {
const a = Math.round(value * 100 + Number.EPSILON);
const b = Math.round((value + Number.EPSILON) * 100);
console.log(JSON.stringify(Object.entries({
value,
value_times_100: value * 100,
epsilon: Number.EPSILON,
expression_a_result: a,
expression_a_rounded_input: value * 100 + Number.EPSILON,
expression_a_midpoint_delta: (value * 100 + Number.EPSILON) - 28.5,
proposed_b_result: b,
proposed_b_rounded_input: (value + Number.EPSILON) * 100,
proposed_b_midpoint_delta: (value + Number.EPSILON) * 100 - 28.5
})));
}
JSRepository: Karanjot786/TermUI
Length of output: 4865
Apply the epsilon before percentage scaling.
Number.EPSILON is added after value * 100, where it is too small to correct half-boundary floating-point errors. For example, 0.285 * 100 rounds down to 28, while adding the epsilon before * 100 rounds it to 29.
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 assigned to pct so Number.EPSILON is added to value before
multiplying by 100, then retain Math.round for the final integer percentage.
| } | ||
| 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
Reject malformed select input.
n is already a number from parseInt, so Number.isNaN(n) does not change the behavior of the previous check. parseInt('2abc', 10) returns 2, so malformed input is still accepted. Parse the complete value and require an integer before selecting an option.
Proposed fix
- const n = parseInt(trimmed, 10);
- if (!Number.isNaN(n) && n >= 1 && n <= choices.length) {
+ const n = Number(trimmed);
+ if (Number.isInteger(n) && n >= 1 && n <= choices.length) {📝 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 (!Number.isNaN(n) && n >= 1 && n <= choices.length) { | |
| const n = Number(trimmed); | |
| if (Number.isInteger(n) && n >= 1 && n <= choices.length) { |
🤖 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` at line 116, Update the select-input validation
around the parsed variable n to reject malformed values such as “2abc”: validate
that the complete user input represents an integer, then retain the existing
1-to-choices.length range check before selecting an option.
| 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
Use a string comparator for dependency names.
a and b are package-name strings, so a - b evaluates to NaN. The sort then preserves insertion order instead of sorting dependencies. The provided test expects @termuijs/core, @termuijs/motion, and @termuijs/widgets, so this change makes that test fail and can make registry output depend on source order.
Use the default lexicographic sort:
Proposed fix
- return [...deps].sort((a, b) => a - b);
+ return [...deps].sort();📝 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(); |
🤖 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 the
registry-building flow to use lexicographic string ordering rather than numeric
subtraction. In the visible sort expression within the dependency collection
logic, use the default string comparator so package names such as
`@termuijs/core`, `@termuijs/motion`, and `@termuijs/widgets` are emitted
deterministically.
1 participant
Description
This PR fixes real bugs found in the codebase:
Number.EPSILONtoMath.round: prevents floating-point drift (e.g.1.005 * 100rounding to 100 instead of 101)..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.Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3510
Summary by CodeRabbit