fix: resolve 4 bugs in termui - #3584
Conversation
📝 WalkthroughWalkthroughThe changes limit hexadecimal entity parsing to eight digits, use strict ChangesParsing and build 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: 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 `@examples/rss-reader/src/index.tsx`:
- Line 30: Update the entity-decoding logic around codePoint to validate the
hexadecimal digit count before parsing; when the entity contains more than eight
digits, return the original match unchanged, otherwise parse the full permitted
value without truncation.
In `@scripts/build-registry.ts`:
- Line 47: Update the sorting logic in collectDeps to use a string comparator
for the returned dependency names instead of numeric subtraction, ensuring
TypeScript compatibility and deterministic alphabetical ordering.
🪄 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: 82161f41-a5df-4bd1-ad50-a02db8be3a18
📒 Files selected for processing (3)
examples/rss-reader/src/index.tsxpackages/ui/src/prompts.tsscripts/build-registry.ts
| 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 | 🟠 Major | ⚡ Quick win
Reject overlong hexadecimal entities instead of truncating them.
The regular expression accepts any number of hexadecimal digits, but entity.slice(2, 10) keeps only the first eight. For example, A becomes U+0004 instead of A. Check the digit count before parsing and return match when the entity exceeds the eight-digit limit.
Proposed fix
- const codePoint = Number.parseInt(entity.slice(2, 10), 16);
+ const digits = entity.slice(2);
+ if (digits.length > 8) {
+ return match;
+ }
+ const codePoint = Number.parseInt(digits, 16);📝 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 codePoint = Number.parseInt(entity.slice(2, 10), 16); | |
| const digits = entity.slice(2); | |
| if (digits.length > 8) { | |
| return match; | |
| } | |
| const codePoint = Number.parseInt(digits, 16); |
🤖 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 count before parsing;
when the entity contains more than eight digits, return the original match
unchanged, otherwise parse the full permitted value without truncation.
| 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:
#!/usr/bin/env bash
set -euo pipefail
if rg -nF 'sort((a, b) => a - b)' scripts/build-registry.ts; then
echo "Numeric comparator still used for string dependencies." >&2
exit 1
fi
rg -n -A12 -B2 'collects unique sorted `@termuijs/`\* specifiers' scripts/build-registry.test.tsRepository: Karanjot786/TermUI
Length of output: 256
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "--- scripts/build-registry.ts ---"
cat -n scripts/build-registry.ts
echo "--- relevant test section ---"
sed -n '110,150p' scripts/build-registry.test.ts | cat -n
echo "--- TypeScript version/config availability ---"
if [ -f package.json ]; then jq '.devDependencies.typescript, .scripts' package.json; fi
if [ -f tsconfig.json ]; then jq '.compilerOptions' tsconfig.json; fiRepository: Karanjot786/TermUI
Length of output: 25168
Use a string comparator for dependency names.
collectDeps returns string[], so a - b is a TS2365 error. If type checking is skipped, the comparator evaluates to NaN, and sort does not guarantee alphabetical dependency order.
Proposed fix
- return [...deps].sort((a, b)章 a - b);
+ return [...deps].sort((a, b) => (a < b ? -1 : a > b ? 1 : 0));🤖 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 logic in
collectDeps to use a string comparator for the returned dependency names instead
of numeric subtraction, ensuring TypeScript compatibility and deterministic
alphabetical ordering.
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.isNaNwithNumber.isNaN: the global version coerces its argument, soisNaN('1')returns false whileNumber.isNaNis strict.parseInt: without10, strings like'0x1F'or'08'parse in unintended bases.Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3583
Summary by CodeRabbit
Bug Fixes
Consistency