fix: resolve 4 bugs in termui - #3660
Conversation
📝 WalkthroughWalkthroughFour example applications update form shortcut handling, RSS hexadecimal parsing, progress rounding, and weather refresh interval management. ChangesForms validation shortcut
RSS entity parsing
Todo progress rounding
Weather refresh interval
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 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: 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: Fix the condition in the keyboard event handler by restoring the
negation before event.ctrl, so the clear-form modal opens only for plain “c” and
Ctrl+C continues through the existing quit branch.
In `@examples/rss-reader/src/index.tsx`:
- Line 30: Update the hexadecimal entity parsing near codePoint so the matched
suffix accepts at most eight hexadecimal digits. Ensure entities with more than
eight digits are returned unchanged rather than truncated, while preserving
decoding for valid entities within the limit.
In `@examples/todo-app/src/index.ts`:
- Line 107: Update the percentage calculation in the progress-rendering logic
around pct so Number.EPSILON is scaled appropriately after value * 100 and
before Math.round. Preserve the existing percentage output while ensuring
boundary values such as 29 / 200 round to 15%.
🪄 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: 2a8d4092-eb08-4c27-bdf0-1f97b2451e50
📒 Files selected for processing (4)
examples/forms-and-validation/src/index.tsxexamples/rss-reader/src/index.tsxexamples/todo-app/src/index.tsexamples/weather/src/index.tsx
| } | ||
|
|
||
| if (event.key === 'c' && event.ctrl === false) { | ||
| if (event.key === 'c' && event.ctrl !) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Restore the missing negation operator.
event.ctrl ! is invalid TypeScript syntax, so the application cannot compile. Use !event.ctrl to open the clear-form modal only for plain c; Ctrl+C remains handled by the quit branch on Lines 121-123.
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, Fix the condition
in the keyboard event handler by restoring the negation before event.ctrl, so
the clear-form modal opens only for plain “c” and Ctrl+C continues through the
existing quit branch.
| 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 hexadecimal entities longer than eight digits instead of truncating them.
Line 28 still matches an unlimited hexadecimal suffix. Line 30 then discards digits after the first eight. For example, A decodes from 00000004 instead of 00000041, which corrupts RSS content.
Check the digit count and return match when it exceeds eight digits, or limit the regular expression to {1,8}.
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 hexadecimal entity
parsing near codePoint so the matched suffix accepts at most eight hexadecimal
digits. Ensure entities with more than eight digits are returned unchanged
rather than truncated, while preserving decoding for valid entities within the
limit.
| const filled = Math.round(barWidth * value); | ||
|
|
||
| const pct = Math.round(value * 100); | ||
| const pct = Math.round(value * 100 + Number.EPSILON); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
node <<'NODE'
const value = 29 / 200;
const current = Math.round(value * 100 + Number.EPSILON);
const proposed = Math.round((value + Number.EPSILON) * 100);
if (current !== 14) {
throw new Error(`Expected current expression to produce 14, got ${current}`);
}
if (proposed !== 15) {
throw new Error(`Expected proposed expression to produce 15, got ${proposed}`);
}
NODERepository: Karanjot786/TermUI
Length of output: 156
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Current expression behavior for boundary values:\n'
node <<'NODE'
const boundaries = [29 / 200, 57 / 200, 85 / 200, 4 / 50, 6 / 50, 29 / 200000000, 1.000000012345678e-8, 2e-14, 1.3333333333333333e-8];
for (const value of boundaries) {
const scaled = value * 100;
const current = Math.round(scaled + Number.EPSILON);
const proposed = Math.round((value + Number.EPSILON) * 100);
console.log({
value,
roundedValue: scaled.toFixed(20),
current,
proposed
});
}
NODE
printf '\nRelevant src/index.ts context:\n'
sed -n '80,120p' examples/todo-app/src/index.tsRepository: Karanjot786/TermUI
Length of output: 2869
Add Number.EPSILON before rounding the percentage.
Number.EPSILON after value * 100 is smaller than the floating-point error at percentage boundaries, so values such as 29 / 200 can still render as 14% instead of 15%.
Proposed fix
- const pct = Math.round(value * 100 + Number.EPSILON);
+ const pct = Math.round((value + Number.EPSILON) * 100);📝 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 pct = Math.round(value * 100 + Number.EPSILON); | |
| const pct = Math.round((value + Number.EPSILON) * 100); |
🤖 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/todo-app/src/index.ts` at line 107, Update the percentage
calculation in the progress-rendering logic around pct so Number.EPSILON is
scaled appropriately after value * 100 and before Math.round. Preserve the
existing percentage output while ensuring boundary values such as 29 / 200 round
to 15%.
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: #3659
Summary by CodeRabbit