fix: resolve 4 bugs in termui - #3444
Conversation
github-actions
Bot
added
type:bug
📝 WalkthroughWalkthroughThe changes clear an existing streaming interval, alter a form shortcut condition, and add explicit numeric comparators for option indices and dependency names. ChangesBehavior corrections
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
🧹 Nitpick comments (1)
packages/ui/src/MultiSelect.ts (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for an option index greater than nine.
The existing test in
packages/ui/src/MultiSelect.test.tsLines 104-114 checks only indices 0-2. That case passes with lexicographic sorting. Add an option at index 10 or greater to verify this numeric comparator.🤖 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, Extend the test covering MultiSelect checked-option ordering in MultiSelect.test.ts to include an option at index 10 or higher, and assert the returned options remain numerically ordered. Keep the existing indices 0–2 while ensuring the test exercises the numeric comparator in MultiSelect’s checked-options mapping.
🤖 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/ai-streaming/src/index.tsx`:
- Line 43: Update AIStreamingApp to store its streaming interval handle on the
component instance instead of the shared window.__interval global. Ensure each
instance clears only its own timer during unmount/destroy, while preserving the
existing interval setup and cleanup behavior.
In `@examples/forms-and-validation/src/index.tsx`:
- Line 125: Update the keyboard condition in the event handler so the clear-form
shortcut checks for c while Ctrl is not pressed by negating event.ctrl,
preserving the existing Ctrl+C quit branch and allowing plain c to open the form
modal.
In `@scripts/build-registry.ts`:
- Line 47: Update the sort comparator in collectDeps to compare dependency
strings alphabetically rather than subtracting them numerically. Preserve the
returned array of unique dependencies while ensuring the comparator satisfies
TypeScript strict-mode typing and produces the expected lexical order.
---
Nitpick comments:
In `@packages/ui/src/MultiSelect.ts`:
- Line 33: Extend the test covering MultiSelect checked-option ordering in
MultiSelect.test.ts to include an option at index 10 or higher, and assert the
returned options remain numerically ordered. Keep the existing indices 0–2 while
ensuring the test exercises the numeric comparator in MultiSelect’s
checked-options mapping.
🪄 Autofix (Beta)
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: f892d73d-0b3c-4f29-a3a3-dccaa0273606
📒 Files selected for processing (4)
examples/ai-streaming/src/index.tsxexamples/forms-and-validation/src/index.tsxpackages/ui/src/MultiSelect.tsscripts/build-registry.ts
| this.addChild(this._streamingText); | ||
|
|
||
| setInterval(() => { | ||
| clearInterval(window.__interval); window.__interval = setInterval(() => { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect: either one live AIStreamingApp instance or instance-specific interval cleanup.
rg -n -C 5 '\bAIStreamingApp\b|__interval|dispose|unmount|destroy|onUnmount' \
examples/ai-streaming packagesRepository: Karanjot786/TermUI
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked matches for AIStreamingApp and window.__interval in examples/ai-streaming only:\n'
rg -n -C 4 'AIStreamingApp|__interval|clearInterval|setInterval' examples/ai-streaming || true
printf '\nindex.tsx with line numbers:\n'
cat -n examples/ai-streaming/src/index.tsx
printf '\nPackage files in examples/ai-streaming:\n'
git ls-files examples/ai-streamingRepository: Karanjot786/TermUI
Length of output: 5944
Keep the streaming interval tied to the app lifecycle.
AIStreamingApp currently uses a single global window.__interval, but TypeScript allows another instance to be created and clear the previous timer. Store the handle on the component and clear it from unmount/destroy so the timer cannot cancel another mounted instance.
🤖 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/ai-streaming/src/index.tsx` at line 43, Update AIStreamingApp to
store its streaming interval handle on the component instance instead of the
shared window.__interval global. Ensure each instance clears only its own timer
during unmount/destroy, while preserving the existing interval setup and cleanup
behavior.
| } | ||
|
|
||
| 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
echo "== files =="
git ls-files | grep -F 'examples/forms-and-validation/src/index.tsx' || true
echo "== target lines =="
if [ -f examples/forms-and-validation/src/index.tsx ]; then
nl -ba examples/forms-and-validation/src/index.tsx | sed -n '105,135p'
fi
echo "== parser semantics probe =="
node - <<'JS'
function parseLike(event) {
// Simulate JavaScript logical condition parsed as event.ctrl! : postfix non-null assertion.
// event.ctrl! evaluates event.ctrl as a truthy/falsy value and does not change it.
return {
eventCtrl: event.ctrl,
eventCtrlBang: event.ctrl, // postfix non-null assertion has the same runtime value as event.ctrl here
}
}
console.log(JSON.stringify({
plainC: parseLike({key: 'c', ctrl: false}),
ctrlC: parseLike({key: 'c', ctrl: true}),
}, null, 2))
JS
echo "== exact token/search =="
rg -n "event\.ctrl\s*!|\bkey === 'c'|\bkey ===\"c\"" examples/forms-and-validation/src/index.tsx || trueRepository: Karanjot786/TermUI
Length of output: 271
🏁 Script executed:
#!/bin/sh
set -euo pipefail
echo "== target lines =="
awk 'NR>=105 && NR<=135 { printf "%6d\t%s\n", NR, $0 }' examples/forms-and-validation/src/index.tsx
echo "== exact token/search =="
grep -nE "event\.ctrl\s*!|key === 'c'|key ===\"c\"" examples/forms-and-validation/src/index.tsx || true
echo "== parse semantics probe =="
node - <<'JS'
function evaluateCondition(event) {
const keyIsC = event.key === 'c'
const ctrlIsTruthy = Boolean(event.ctrl) // event.ctrl! has the same runtime boolean semantics as event.ctrl
return keyIsC && ctrlIsTruthy
}
console.log(JSON.stringify({
plainC: { eventKey: 'c', eventCtrl: false, conditionTrue: evaluateCondition({key: 'c', ctrl: false}) },
ctrlC: { eventKey: 'c', eventCtrl: true, conditionTrue: evaluateCondition({key: 'c', ctrl: true}) },
}, null, 2))
JSRepository: Karanjot786/TermUI
Length of output: 1716
Negate event.ctrl to restore the clear-form shortcut.
event.ctrl ! is parsed as the postfix non-null assertion event.ctrl!, so the condition only matches when Ctrl is already held. Since Ctrl+C is handled by the quit branch above, plain c does not open the form modal. Change it to !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, Update the keyboard
condition in the event handler so the clear-form shortcut checks for c while
Ctrl is not pressed by negating event.ctrl, preserving the existing Ctrl+C quit
branch and allowing plain c to open the form modal.
| 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
if rg -n 'sort\(\(a, b\) => a - b\)' scripts/build-registry.ts; then
echo "Invalid string subtraction comparator remains." >&2
exit 1
fi
rg -n -C 3 'collectDeps|sort\(' \
scripts/build-registry.ts \
scripts/build-registry.test.tsRepository: Karanjot786/TermUI
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '1,90p' scripts/build-registry.ts 2>/dev/null || true
echo
echo "== test excerpts =="
sed -n '110,150p' scripts/build-registry.test.ts 2>/dev/null || true
echo
echo "== TypeScript availability =="
if command -v tsc >/dev/null 2>&1 && command -v ts-node >/dev/null 2>&1; then
tmp="$(mktemp -d)"
yes 'n' | npm init - >/tmp/npm-init.log 2>&1 || true
npm install typescript --save-dev >/tmp/npm-install.log 2>&1 || true
cat > "$tmp/test-string-compare.ts" <<'TS'
type SetLike<T> = Iterable<T>;
declare const deps: Set<string>;
const result = [...deps].sort((a: string, b: string) => a - b);
TS
npx tsc --noEmit --strict "$tmp/test-string-compare.ts" 2>&1 | sed -n '1,40p'
exit 0
else
echo "tsc/ts-node not available"
fi
echo
echo "== direct JavaScript probe =="
node - <<'JS'
const deps = new Set(['pkg-b', 'pkg-a', 'pkg-c']);
const result = [...deps].sort((a, b) => a - b);
console.log(JSON.stringify(Array.from(result)));
JSRepository: Karanjot786/TermUI
Length of output: 5345
🌐 Web query:
Node.js V table function object arithmetic comparison string subtract returns NaN spec
💡 Result:
In JavaScript, and by extension Node.js, the subtraction operator (-) returns NaN (Not-a-Number) when applied to strings that cannot be converted into valid numbers [1][2]. This occurs because the subtraction operator is strictly for numeric or BigInt operations, prompting the engine to perform implicit type conversion using the ToNumber abstract operation on both operands [1][3][4]. When the subtraction operator encounters a string, it attempts to convert that string to a number [1][2]. If the string does not contain a valid numeric representation (e.g., "hello"), the conversion result is NaN [1][5]. Because any mathematical operation performed with NaN results in NaN, the entire expression evaluates to NaN [5][6]. This behavior is explicitly defined in the ECMAScript Language Specification [7][8]. The evaluation of the subtraction operator invokes the EvaluateStringOrNumericBinaryExpression operation [7][8], which eventually relies on the ToNumber abstract operation [4]. Key points regarding this behavior: 1. Implicit Conversion: The subtraction operator triggers an attempt to convert operands to numbers (Number type) [1][4]. 2. NaN Result: If a string is not a valid number, it converts to NaN [1][6]. Mathematically, x - NaN or NaN - y results in NaN [5][6]. 3. Relational Comparisons: Unlike subtraction, relational operators (like < or >) also trigger conversion to numbers but treat operands differently if both are strings, performing a lexicographical (alphabetical) comparison instead of a numeric one [9][10]. If at least one operand is NaN, relational comparisons return false [9][6]. 4. TypeOf NaN: Note that NaN is a value of the Number type, which is why the result of an invalid string subtraction is considered a number type in JavaScript [5]. For predictable results, it is recommended to explicitly convert inputs to numbers using Number, parseInt, or parseFloat before performing arithmetic, rather than relying on implicit coercion [11][2].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Subtraction
- 2: https://wsvincent.com/javascript-arithmetic-operators/
- 3: https://stackoverflow.com/questions/47026942/minus-operator-on-strings-in-javascript
- 4: https://tc39.es/ecma262/2026/multipage/abstract-operations.html
- 5: https://stackoverflow.com/questions/37301296/why-is-string-string-nan
- 6: https://stackoverflow.com/questions/47709665/can-any-one-explain-me-how-does-the-string-and-number-comparison-work-in-javascr
- 7: https://tc39.es/ecma262/2026/multipage/ecmascript-language-expressions.html
- 8: https://tc39.es/ecma262/multipage/ecmascript-language-expressions.html
- 9: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Less_than
- 10: https://transang.me/how-does-javascript-do-a-relational-comparison/
- 11: https://stackoverflow.com/questions/37646607/javascript-subtraction-returns-nan
Replace the numeric comparator with a string comparator.
collectDeps returns strings from Set<string>, and a - b violates TypeScript strict-mode type checking. It also returns NaN, so the dependency list does not sort alphabetically as the test expects.
Proposed fix
- 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 sort comparator in
collectDeps to compare dependency strings alphabetically rather than subtracting
them numerically. Preserve the returned array of unique dependencies while
ensuring the comparator satisfies TypeScript strict-mode typing and produces the
expected lexical order.
Source: Coding guidelines
1 participant
Description
This PR fixes real bugs found in the codebase:
x === trueis equivalent tox(andx === falseto!x), and shorter to read..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.Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3443
Summary by CodeRabbit