fix: resolve 4 bugs in termui - #3719
Conversation
github-actions
Bot
added
type:bug
📝 WalkthroughWalkthroughThe changes update keyboard shortcut conditions, limit hexadecimal entity parsing to eight digits, enforce decimal tab parsing, and apply numeric sorting during ChangesParsing and comparison corrections
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 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/forms-and-validation/src/index.tsx`:
- Line 125: In the clear-form keyboard shortcut condition, replace the non-null
assertion on event.ctrl with logical negation so the modal is shown for plain
“c” and Ctrl+C remains handled by the earlier path. Update the condition in the
visible event-key handler without changing the surrounding shortcut behavior.
In `@examples/rss-reader/src/index.tsx`:
- Line 30: Update the entity-decoding logic around codePoint to validate the
hexadecimal digit length before parsing; when the entity exceeds eight digits,
return match unchanged, otherwise preserve the existing Number.parseInt
behavior.
In `@packages/ui/src/TreeSelect.ts`:
- Around line 185-186: Update _valuesEqual to define one named comparator and
pass it to both sortedA and sortedB, ensuring numeric and nonnumeric string
values are ordered consistently without NaN comparisons. Add regression tests
covering both numeric and nonnumeric string arrays.
🪄 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: ab2b69cd-21df-4bc2-b90e-b8a3287a317b
📒 Files selected for processing (4)
examples/forms-and-validation/src/index.tsxexamples/rss-reader/src/index.tsxexamples/showcase/src/index.tsxpackages/ui/src/TreeSelect.ts
| } | ||
|
|
||
| if (event.key === 'c' && event.ctrl === false) { | ||
| if (event.key === 'c' && event.ctrl !) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="examples/forms-and-validation/src/index.tsx"
rg -n -C 3 "event\.key === 'c'|event\.ctrl" "$file"Repository: Karanjot786/TermUI
Length of output: 906
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const cases = [
{ key: 'c', ctrl: false },
{ key: 'c', ctrl: true },
{ key: 'C', ctrl: false },
];
for (const event of cases) {
const firstCondition = event.key === 'q' || (event.ctrl && event.key === 'c');
// Simulate TS "event.ctrl !" as runtime event.ctrl with no boolean inversion.
const secondCondition = event.key === 'c' && event.ctrl;
console.log(JSON.stringify({
event,
branch: firstCondition ? 'return false' : secondCondition ? 'show modal' : 'char input',
}));
}
JSRepository: Karanjot786/TermUI
Length of output: 328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="examples/forms-and-validation/src/index.tsx"
sed -n '112,132p' "$file"Repository: Karanjot786/TermUI
Length of output: 840
Use logical negation for the clear-form shortcut.
event.ctrl ! asserts non-null, so it still checks event.ctrl. The path consumes Ctrl+C earlier, and plain c now reaches character input, so this.modal.show() never runs.
Replace it with !event.ctrl.
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 clear-form
keyboard shortcut condition, replace the non-null assertion on event.ctrl with
logical negation so the modal is shown for plain “c” and Ctrl+C remains handled
by the earlier path. Update the condition in the visible event-key handler
without changing the surrounding shortcut behavior.
| return value.replace(/&(#x?[0-9a-fA-F]+|[a-zA-Z]+);/g, (match, entity: string) => { | ||
| if (entity.startsWith('#x')) { | ||
| const codePoint = Number.parseInt(entity.slice(2), 16); | ||
| const codePoint = Number.parseInt(entity.slice(2, 10), 16); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject overlong hexadecimal entities instead of truncating them.
The regular expression accepts all hexadecimal digits, but Line 30 parses only the first eight. For example, П decodes as A and silently drops the final F.
Validate the digit length before parsing. Return match when the entity exceeds the supported limit.
🤖 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` at line 30, Update the entity-decoding
logic around codePoint to validate the hexadecimal digit length before parsing;
when the entity exceeds eight digits, return match unchanged, otherwise preserve
the existing Number.parseInt behavior.
| const sortedA = [...a].sort((a, b) => a - b); | ||
| const sortedB = [...b].sort(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
npx tsc --noEmit --pretty falseRepository: Karanjot786/TermUI
Length of output: 5708
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- locate files ---\n'
git ls-files | rg '(^|/)TreeSelect\.(ts|test\.ts)$|(^|/)tsconfig\.json$|packages/ui/src/index\.ts$' || true
printf '\n--- TreeSelect.ts outline ---\n'
ast-grep outline packages/ui/src/TreeSelect.ts --view expanded || true
printf '\n--- TreeSelect.ts relevant lines 140-210 ---\n'
cat -n packages/ui/src/TreeSelect.ts | sed -n '140,210p'
printf '\n--- TreeSelect.test.ts relevant lines ---\n'
cat -n packages/ui/src/TreeSelect.test.ts | sed -n '1,220p'
printf '\n--- package/compiler flags references ---\n'
for f in $(git ls-files '*tsconfig*.json'); do
echo "--- $f ---"
cat -n "$f" | sed -n '1,180p'
done
printf '\n--- string subtraction runtime probe ---\n'
node - <<'JS'
const inputs = [
['10', '2'],
['2', '10'],
['10', '10'],
['s', 'a'],
['a', 's']
];
for (const [a, b] of inputs) {
const diff = a - b;
console.log(JSON.stringify([a, b, diff, diff === diff]))
}
JSRepository: Karanjot786/TermUI
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- locate files ---'
git ls-files | rg '(^|/)TreeSelect\.(ts|test\.ts)$|(^|/)tsconfig\.json$|packages/ui/src/index\.ts$' || true
printf '%s\n' ''
printf '%s\n' '--- TreeSelect.ts outline ---'
ast-grep outline packages/ui/src/TreeSelect.ts --view expanded || true
printf '%s\n' ''
printf '%s\n' '--- TreeSelect.ts relevant lines 140-210 ---'
cat -n packages/ui/src/TreeSelect.ts | sed -n '140,210p'
printf '%s\n' ''
printf '%s\n' '--- TreeSelect.test.ts relevant lines ---'
cat -n packages/ui/src/TreeSelect.test.ts | sed -n '1,220p'
printf '%s\n' ''
printf '%s\n' '--- package/compiler flags references ---'
for f in $(git ls-files '*tsconfig*.json'); do
echo "--- $f ---"
cat -n "$f" | sed -n '1,180p'
done
printf '%s\n' ''
printf '%s\n' '--- string subtraction runtime probe ---'
node - <<'JS'
const inputs = [
['10', '2'],
['2', '10'],
['10', '10'],
['s', 'a'],
['a', 's']
];
for (const [a, b] of inputs) {
const diff = a - b;
console.log(JSON.stringify([a, b, diff, diff === diff]))
}
JSRepository: Karanjot786/TermUI
Length of output: 29869
Use one comparator for _valuesEqual.
a - b can produce NaN for nonnumeric strings like 's' and 'a', and sortedB still uses lexicographic order. Apply the same named comparator to both sorted arrays, and include regression tests for numeric and nonnumeric string values.
🤖 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 to
define one named comparator and pass it to both sortedA and sortedB, ensuring
numeric and nonnumeric string values are ordered consistently without NaN
comparisons. Add regression tests covering both numeric and nonnumeric string
arrays.
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.parseInt: without10, strings like'0x1F'or'08'parse in unintended bases.parseInt: without10, strings like'0x1F'or'08'parse in unintended bases.Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3718
Summary by CodeRabbit