fix: resolve 4 bugs in termui - #3671
Conversation
📝 WalkthroughWalkthroughThe pull request applies targeted fixes for decimal parsing, floating-point rounding, numeric array comparison, and rejected child-process promise handling. The reload rejection callback currently contains invalid TypeScript syntax. ChangesCorrectness fixes
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.6)packages/dev-server/src/server.tsFile contains syntax errors that prevent linting: Line 383: Expected a parenthesis '(' but instead found '=>'. 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
🤖 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 `@packages/dev-server/src/server.ts`:
- Line 383: Fix the invalid catch callback in the exitedPromise handling by
accepting its rejection value and passing that value to console.error,
preserving the existing await and error-reporting behavior.
In `@packages/ui/src/TreeSelect.ts`:
- Around line 185-186: Update _valuesEqual so sortedA and sortedB both use the
same comparator. Preserve numeric ordering for numeric values and provide a
deterministic fallback for non-numeric values, ensuring equivalent
numeric-string arrays in different orders compare equal without changing the
existing equality behavior.
🪄 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: 720a039f-5ce2-4487-b6b8-eafa2195db4f
📒 Files selected for processing (4)
examples/widget-gallery/src/index.tspackages/dev-server/src/server.tspackages/ui/src/Switch.tspackages/ui/src/TreeSelect.ts
| this._killChild(); | ||
|
|
||
| await exitedPromise.catch(() => {}); | ||
| await exitedPromise.catch( => console.error()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix the invalid .catch callback before merge.
Line [383] does not parse because the callback parameter is missing. This blocks the TypeScript build. Pass the rejection value to console.error; otherwise the error remains discarded.
Proposed fix
- await exitedPromise.catch( => console.error());
+ await exitedPromise.catch((error) => {
+ console.error(error);
+ });📝 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.
| await exitedPromise.catch( => console.error()); | |
| await exitedPromise.catch((error) => { | |
| console.error(error); | |
| }); |
🧰 Tools
🪛 Biome (2.5.6)
[error] 383-383: Expected a parenthesis '(' but instead found '=>'.
(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/dev-server/src/server.ts` at line 383, Fix the invalid catch
callback in the exitedPromise handling by accepting its rejection value and
passing that value to console.error, preserving the existing await and
error-reporting behavior.
Source: Linters/SAST tools
| const sortedA = [...a].sort((a, b) => a - b); | ||
| const sortedB = [...b].sort(); |
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 '(^|/)TreeSelect(\.ts|\.test\.ts|\.tsx)$|packages/ui/src/TreeSelect'
echo "== outline =="
if [ -f packages/ui/src/TreeSelect.ts ]; then
ast-grep outline packages/ui/src/TreeSelect.ts || true
fi
echo "== relevant lines =="
if [ -f packages/ui/src/TreeSelect.ts ]; then
sed -n '150,210p' packages/ui/src/TreeSelect.ts | cat -n
fi
echo "== tests with values =="
if [ -f packages/ui/src/TreeSelect.test.ts ]; then
rg -n -C 3 "TreeSelect|_valuesEqual|values=|setKey|handleKey|sort|s|a|10|2" packages/ui/src/TreeSelect.test.ts || true
fi
echo "== package script availability =="
ls -la packages/ui/package.json 2>/dev/null || trueRepository: Karanjot786/TermUI
Length of output: 6604
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all _valuesEqual usages =="
rg -n "_valuesEqual|selectedValues|selectedValues.set|selectedValues.has|selectedValues.add" packages/ui/src packages --glob '*.ts' --glob '*.tsx' || true
echo "== source around calls =="
sed -n '1,120p' packages/ui/src/TreeSelect.ts | cat -nRepository: Karanjot786/TermUI
Length of output: 12579
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
function valuesEqualNumeric(a, b) {
const sortedA = [...a].sort((a, b) => a - b);
const sortedB = [...b].sort();
if (sortedA.length !== sortedB.length) return false;
for (let i = 0; i < sortedA.length; i++) if (sortedA[i] !== sortedB[i]) return false;
return true;
}
function valuesEqualSameComparator(a, b) {
const compare = (left, right) => {
const leftNumber = Number(left);
const rightNumber = Number(right);
if (Number.isFinite(leftNumber) && Number.isFinite(rightNumber)) return leftNumber - rightNumber;
return left.localeCompare(right);
};
const sortedA = [...a].sort(compare);
const sortedB = [...b].sort(compare);
if (sortedA.length !== sortedB.length) return false;
for (let i = 0; i < sortedA.length; i++) if (sortedA[i] !== sortedB[i]) return false;
return true;
}
for (const pairs of [
[['10','2'], ['2','10']],
[['a','b'], ['b','a']],
[['a','10'], ['10','a']]
]) {
console.log(JSON.stringify({ pairs, current: valuesEqualNumeric(...pairs), sameComparator: valuesEqualSameComparator(...pairs) }));
}
JSRepository: Karanjot786/TermUI
Length of output: 369
Use one comparator for _valuesEqual.
TreeSelect.test.ts uses non-numeric values ('s', 'a'), but _valuesEqual sorts next with the default locale comparator while still passing a callback to previous. If the values are numeric strings in a different order, such as ['10', '2'] versus ['2', '10'], the arrays compare unequal and causes extra change events. Use the same comparator for both arrays, including a deterministic non-numeric fallback.
🤖 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` around lines 185 - 186, Update _valuesEqual so
sortedA and sortedB both use the same comparator. Preserve numeric ordering for
numeric values and provide a deterministic fallback for non-numeric values,
ensuring equivalent numeric-string arrays in different orders compare equal
without changing the existing equality behavior.
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.Number.EPSILONtoMath.round: prevents floating-point drift (e.g.1.005 * 100rounding to 100 instead of 101).parseInt: without10, strings like'0x1F'or'08'parse in unintended bases.Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3670
Summary by CodeRabbit