fix: resolve 4 bugs in termui - #3634
Conversation
github-actions
Bot
added
area:examples
📝 WalkthroughWalkthroughThe pull request applies four targeted fixes: limits hexadecimal entity parsing, prevents duplicate weather refresh timers, corrects switch knob rounding, and simplifies confirmation prompt hint selection. ChangesCorrectness fixes
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🧪 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: 1
🧹 Nitpick comments (1)
packages/ui/src/Switch.ts (1)
117-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the rounding boundary.
The existing
packages/ui/src/Switch.test.tsrendering tests do not cover_animProgressvalues just below the0.5and1.5rounding thresholds. Add a deterministic test that drives those values and verifies the rendered knob position.🤖 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/Switch.ts` at line 117, Add a deterministic regression test in the Switch rendering tests that drives _animProgress to values just below 0.5 and 1.5, then verifies the rendered knob position produced by the knobPos calculation. Cover both rounding boundaries without changing the implementation in Switch.
🤖 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`:
- Around line 30-31: Update the entity decoder around codePoint so it rejects
hexadecimal entities containing more than eight digits by returning match,
rather than truncating with slice(2, 10). Validate that the parsed value is
within the Unicode range 0x0000–0x10FFFF before calling String.fromCodePoint,
preserving match for invalid or out-of-range entities.
---
Nitpick comments:
In `@packages/ui/src/Switch.ts`:
- Line 117: Add a deterministic regression test in the Switch rendering tests
that drives _animProgress to values just below 0.5 and 1.5, then verifies the
rendered knob position produced by the knobPos calculation. Cover both rounding
boundaries without changing the implementation in Switch.
🪄 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: 403a1b16-9b6c-436d-b691-283138d95c72
📒 Files selected for processing (4)
examples/rss-reader/src/index.tsxexamples/weather/src/index.tsxpackages/ui/src/Switch.tspackages/ui/src/prompts.ts
| const codePoint = Number.parseInt(entity.slice(2, 10), 16); | ||
| return Number.isFinite(codePoint) ? String.fromCodePoint(codePoint) : match; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject overlong hexadecimal entities instead of truncating them.
The regular expression still captures all hexadecimal digits. entity.slice(2, 10) discards digits after the first eight. For К, the decoder returns A instead of preserving the invalid entity. A first-eight-digit value can also exceed 0x10FFFF, so String.fromCodePoint can still throw RangeError.
If the entity has more than eight digits, return match. Also validate the Unicode range before calling String.fromCodePoint.
Proposed fix
- const codePoint = Number.parseInt(entity.slice(2, 10), 16);
- return Number.isFinite(codePoint) ? String.fromCodePoint(codePoint) : match;
+ const digits = entity.slice(2);
+ if (digits.length > 8) return match;
+ const codePoint = Number.parseInt(digits, 16);
+ return Number.isFinite(codePoint) && codePoint <= 0x10ffff
+ ? String.fromCodePoint(codePoint)
+ : match;📝 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); | |
| return Number.isFinite(codePoint) ? String.fromCodePoint(codePoint) : match; | |
| const digits = entity.slice(2); | |
| if (digits.length > 8) return match; | |
| const codePoint = Number.parseInt(digits, 16); | |
| return Number.isFinite(codePoint) && codePoint <= 0x10ffff | |
| ? String.fromCodePoint(codePoint) | |
| : match; |
🤖 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` around lines 30 - 31, Update the entity
decoder around codePoint so it rejects hexadecimal entities containing more than
eight digits by returning match, rather than truncating with slice(2, 10).
Validate that the parsed value is within the Unicode range 0x0000–0x10FFFF
before calling String.fromCodePoint, preserving match for invalid or
out-of-range entities.
1 participant
Description
This PR fixes real bugs found in the codebase:
parseInt: without10, strings like'0x1F'or'08'parse in unintended bases.Number.EPSILONtoMath.round: prevents floating-point drift (e.g.1.005 * 100rounding to 100 instead of 101).x === trueis equivalent tox(andx === falseto!x), and shorter to read.Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3633
Summary by CodeRabbit