fix: resolve 4 bugs in termui - #3653
Conversation
github-actions
Bot
added
type:bug
📝 WalkthroughWalkthroughThe changes fix three independent behaviors: weather interval replacement, strict numeric validation in select prompts, and numeric ordering of collected registry dependencies. ChangesWeather timer management
Prompt input validation
Registry 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: 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/ui/src/prompts.ts`:
- Line 116: Update the prompt selection validation around the numeric parsing
condition to validate the complete raw input before accepting it, rather than
relying on parseInt’s partial parsing. Require a valid integer index within 1
through choices.length, rejecting values such as “2abc” and “2.5”; add
regression tests covering malformed-prefix and decimal inputs.
In `@scripts/build-registry.ts`:
- Line 47: Update the sorting comparator in collectDeps() to compare dependency
specifier strings lexically rather than subtracting them numerically. Preserve
the returned string[] and ensure the ordering matches the lexical order expected
by the existing tests.
🪄 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: c96cc369-8a35-4346-bce7-9efa57987373
📒 Files selected for processing (3)
examples/weather/src/index.tsxpackages/ui/src/prompts.tsscripts/build-registry.ts
| } | ||
| 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 | 🔴 Critical | ⚡ Quick win
Validate the complete input before accepting a selection.
Number.isNaN(n) is equivalent to the previous check here because n is already a number from parseInt. It does not reject inputs with numeric prefixes. For example, 2abc and 2.5 both parse as 2, so the prompt can select the wrong option.
Parse the complete input and require an integer index. Add regression tests for malformed and decimal input.
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) {🤖 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 prompt selection
validation around the numeric parsing condition to validate the complete raw
input before accepting it, rather than relying on parseInt’s partial parsing.
Require a valid integer index within 1 through choices.length, rejecting values
such as “2abc” and “2.5”; add regression tests covering malformed-prefix and
decimal inputs.
| 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
echo "== files =="
git ls-files | rg '(^|/)build-registry(\.test)?\.ts$' || true
echo "== build-registry.ts =="
if [ -f scripts/build-registry.ts ]; then
nl -ba scripts/build-registry.ts | sed -n '1,90p'
fi
echo "== related tests =="
if [ -f scripts/build-registry.test.ts ]; then
nl -ba scripts/build-registry.test.ts | sed -n '1,180p'
fi
echo "== tsconfig(s) =="
fd -a 'tsconfig.*\.json$' . -d 3 | sort | sed 's#^\./##' | while read -r f; do
echo "--- $f"
nl -ba "$f" | sed -n '1,140p'
done
echo "== TypeScript availability and arithmetic probe =="
if command -v tsc >/dev/null 2>&1; then
echo "tsc version:"
tsc --version
tmp="$(mktemp -d)"
cat > "$tmp/probe.ts" <<'TS'
const deps: string[] = ['apple', 'banana', 'apricot'];
deps.sort((a: string, b: string) => a - b);
TS
(tsc --noEmit --strict "$tmp/probe.ts" 2>&1 | sed -n '1,120p') || true
else
node - <<'JS'
const deps = ['apple', 'banana', 'apricot'];
const out = [...deps].sort((a, b) => a - b);
console.log(JSON.stringify(out));
JS
fiRepository: Karanjot786/TermUI
Length of output: 289
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== build-registry.ts =="
if [ -f scripts/build-registry.ts ]; then
cat -n scripts/build-registry.ts | sed -n '1,90p'
fi
echo "== related tests =="
if [ -f scripts/build-registry.test.ts ]; then
cat -n scripts/build-registry.test.ts | sed -n '1,180p'
fi
echo "== tsconfig(s) =="
find . -maxdepth 3 -name 'tsconfig*.json' -print | sort | while read -r f; do
echo "--- $f"
cat -n "$f" | sed -n '1,160p'
done
echo "== TypeScript availability and arithmetic probe =="
if command -v tsc >/dev/null 2>&1; then
echo "tsc version:"
tsc --version
tmp="$(mktemp -d)"
cat > "$tmp/probe.ts" <<'TS'
const deps: string[] = ['apple', 'banana', 'apricot'];
deps.sort((a: string, b: string) => a - b);
TS
(tsc --noEmit --strict "$tmp/probe.ts" 2>&1 | sed -n '1,120p') || true
else
echo "tsc not available"
node - <<'JS'
const deps = ['apple', 'banana', 'apricot'];
const out = [...deps].sort((a, b) => a - b);
console.log(JSON.stringify(out));
JS
fiRepository: Karanjot786/TermUI
Length of output: 34283
Use a string comparator for dependency specifiers.
collectDeps() returns string[], but a - b is not valid TypeScript arithmetic for strings. If emitted, NaN values also leave the dependency array unsorted instead of matching the lexical order expected by scripts/build-registry.test.ts:127-139.
- 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 sorting comparator in
collectDeps() to compare dependency specifier strings lexically rather than
subtracting them numerically. Preserve the returned string[] and ensure the
ordering matches the lexical order expected by the existing tests.
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.isNaNwithNumber.isNaN: the global version coerces its argument, soisNaN('1')returns false whileNumber.isNaNis strict..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: #3652
Summary by CodeRabbit