fix: resolve 4 bugs in termui - #3643
Conversation
github-actions
Bot
added
type:bug
📝 WalkthroughWalkthroughThe changes correct whitespace-only paragraph detection, floating-point percentage rounding, and lexicographic sorting of numeric option values in example applications and UI components. ChangesCorrectness fixes
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)
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: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a two-digit index regression test.
The existing test selects only indices 0, 1, and 2. It passes with both numeric and lexicographic sorting. Add a case that selects index 10 before index 2 and asserts that
selectedOptionsfollows option order.🤖 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 test for the MultiSelect selectedOptions behavior that selects index 10 before index 2 and verifies the result follows option order, ensuring numeric sorting rather than lexicographic 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/pomodoro-timer/src/index.tsx`:
- Line 185: Add Number.EPSILON to the normalized value before scaling in the
label calculation that uses _showLabel and _value, so boundary values such as
0.145 display as 15%. Add a regression test covering this percentage-rounding
case.
In `@packages/ui/src/TreeSelect.ts`:
- Line 185: Update _valuesEqual to use one typed numeric comparator for both
sorted arrays, converting string elements to numbers before comparison. Apply
that comparator to sortedA and sortedB so equivalent numeric sets compare equal
and strict TypeScript checking succeeds.
---
Nitpick comments:
In `@packages/ui/src/MultiSelect.ts`:
- Line 33: Add a regression test for the MultiSelect selectedOptions behavior
that selects index 10 before index 2 and verifies the result follows option
order, ensuring numeric sorting rather than lexicographic 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: ca56ecfe-b835-48cb-8ff5-7c0ad5e99b16
📒 Files selected for processing (4)
examples/chat-app/src/index.tsxexamples/pomodoro-timer/src/index.tsxpackages/ui/src/MultiSelect.tspackages/ui/src/TreeSelect.ts
| const attrs = styleToCellAttrs(this._style); | ||
|
|
||
| const label = this._showLabel ? ` ${Math.round(this._value * 100)}%` : ''; | ||
| const label = this._showLabel ? ` ${Math.round(this._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
node <<'NODE'
const value = 0.145;
const current = Math.round(value * 100 + Number.EPSILON);
const corrected = Math.round((value + Number.EPSILON) * 100);
if (current !== 14 || corrected !== 15) {
throw new Error(`Unexpected results: current=${current}, corrected=${corrected}`);
}
NODERepository: Karanjot786/TermUI
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Current expression values:\n'
node <<'NODE'
const value = 0.145;
const current = Math.round(value * 100 + Number.EPSILON);
const corrected = Math.round((value + Number.EPSILON) * 100);
console.log('0.145 * 100 + EPSILON =', value * 100 + Number.EPSILON);
console.log('current =', current);
console.log('corrected =', corrected);
NODE
printf '\nOther small percentages nearby half-centuries:\n'
node <<'NODE'
for (const value of [0.005, 0.015, 0.025, 0.035, 0.045, 0.055, 0.065, 0.075, 0.085, 0.095, 0.145, 0.155, 0.245]) {
const before = value * 100 + Number.EPSILON;
const current = Math.round(before);
const corrected = Math.round((value + Number.EPSILON) * 100);
console.log(`${value}: ${before} => ${current}, corrected => ${corrected}`);
}
NODE
printf '\nLarger values at similar fractional boundaries:\n'
node <<'DEV'
const values = [99.945, 99.955];
for (const value of values) {
const before = value * 100 + Number.EPSILON;
const current = Math.round(before);
const corrected = Math.round((value + Number.EPSILON) * 100);
console.log(`${value}: ${before} => ${current}, corrected => ${corrected}`);
}
NODERepository: Karanjot786/TermUI
Length of output: 1568
Add Number.EPSILON before scaling.
Add Number.EPSILON before multiplying by 100 so values like 0.145 round to 15% instead of 14%. Add a regression test for this boundary case.
🤖 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/pomodoro-timer/src/index.tsx` at line 185, Add Number.EPSILON to the
normalized value before scaling in the label calculation that uses _showLabel
and _value, so boundary values such as 0.145 display as 15%. Add a regression
test covering this percentage-rounding case.
| 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
if rg -nF 'sort((a, b) => a - b)' packages/ui/src/TreeSelect.ts; then
echo "TreeSelect still subtracts string operands directly."
exit 1
fi
sed -n '183,190p' packages/ui/src/TreeSelect.tsRepository: Karanjot786/TermUI
Length of output: 263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TreeSelect relevant lines =="
nl -ba packages/ui/src/TreeSelect.ts | sed -n '140,205p'
echo
echo "== TypeScript configuration / strict mode =="
nl -ba tsconfig.json 2>/dev/null | sed -n '1,220p' || true
rg -n '"strict"|"strict:' tsconfig*.json package.json 2>/dev/null || true
echo
echo "== TypeScript compiler availability and type probe =="
node - <<'JS'
const { execSync } = require('node:child_process');
try {
console.log("tsc_version", execSync('tsc --version', { encoding: 'utf8' }).trim());
} catch (e) {
console.log("tsc unavailable");
}
JSRepository: Karanjot786/TermUI
Length of output: 228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TreeSelect relevant lines =="
awk '{printf "%6d\t%s\n", NR, $0}' packages/ui/src/TreeSelect.ts | sed -n '140,205p'
echo
echo "== TypeScript configuration / strict mode =="
for f in tsconfig.json packages/tsconfig.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
awk '{printf "%6d\t%s\n", NR, $0}' "$f" | sed -n '1,220p'
fi
done
grep -RIn '"strict"|"strict:' tsconfig*.json package.json .github 2>/dev/null || true
echo
echo "== TypeScript compiler availability and type probe =="
if command -v tsc >/dev/null 2>&1; then
tsc --version
mkdir -p /tmp/ts-probe
cat >/tmp/ts-probe/check.ts <<'TS'
const valuesEqual = (a: string[], b: string[]) => {
const sortedA = [...a].sort((a, b) => a - b);
const sortedB = [...b].sort();
return JSON.stringify(sortedA) === JSON.stringify(sortedB);
};
TS
tsc --noEmit --strict /tmp/ts-probe/check.ts 2>&1 || true
else
echo "tsc unavailable"
fi
echo
echo "== Runtime probe for numeric vs lexicographic sort equality =="
node - <<'JS'
const inputs = [['2', '10'], ['10', '2']];
function current(a, b) {
const sortedA = [...a].sort((a, b) => a - b);
const sortedB = [...b].sort();
return JSON.stringify(sortedA) === JSON.stringify(sortedB);
}
function numericBoth(a, b) {
const compare = (left, right) => Number(left) - Number(right);
const sortedA = [...a].sort(compare);
const sortedB = [...b].sort(compare);
return JSON.stringify(sortedA) === JSON.stringify(sortedB);
}
console.log('current_equal', current(...inputs));
console.log('numeric_both_equal', numericBoth(...inputs));
JSON.stringify([...['2', '10']].sort((a, b) => a - b))
JSRepository: Karanjot786/TermUI
Length of output: 2936
Use one typed numeric comparator for both arrays.
_valuesEqual receives string[], so subtracting a - b breaks strict TypeScript checking. sortedB stays lexicographic, so equivalent sets such as ['2', '10'] and ['10', '2'] can compare unequal. Use the same numeric comparator for both arrays.
Proposed fix
- const sortedA = [...a].sort((a, b) => a - b);
- const sortedB = [...b].sort();
+ const numericCompare = (left: string, right: string) => Number(left) - Number(right);
+ const sortedA = [...a].sort(numericCompare);
+ const sortedB = [...b].sort(numericCompare);📝 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 sortedA = [...a].sort((a, b) => a - b); | |
| const numericCompare = (left: string, right: string) => Number(left) - Number(right); | |
| const sortedA = [...a].sort(numericCompare); | |
| const sortedB = [...b].sort(numericCompare); |
🧰 Tools
🪛 GitHub Actions: CI / 0_build-and-test.txt
[error] 185-185: TypeScript error TS2362: The left-hand side of an arithmetic operation must be of type any, number, bigint, or an enum type during the tsup declaration build.
[error] 185-185: TypeScript error TS2363: The right-hand side of an arithmetic operation must be of type any, number, bigint, or an enum type. The 'tsup' build failed with exit code 1.
🪛 GitHub Actions: CI / build-and-test
[error] 185-185: TypeScript errors TS2362 and TS2363: arithmetic operation operands must be any, number, bigint, or an enum type. DTS build failed during 'tsup' and the build script exited with code 1.
🤖 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 one
typed numeric comparator for both sorted arrays, converting string elements to
numbers before comparison. Apply that comparator to sortedA and sortedB so
equivalent numeric sets compare equal and strict TypeScript checking succeeds.
Source: Coding guidelines
1 participant
Description
This PR fixes real bugs found in the codebase:
trim()to''misses whitespace-only input;.trim().length === 0is explicit..sort()coerces elements to strings, so[10, 9, 2]sorts as[10, 2, 9]; numeric comparator sorts correctly..sort()coerces elements to strings, so[10, 9, 2]sorts as[10, 2, 9]; numeric comparator sorts correctly.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: #3642
Summary by CodeRabbit