fix: resolve 4 bugs in termui - #3595
Closed
saurabhhhcodes wants to merge 1 commit into
Closed
Conversation
📝 WalkthroughWalkthroughThree examples receive small correctness updates for keyboard condition checks, floating-point percentage rounding, and radix-10 integer parsing. ChangesExample correctness updates
Estimated code review effort: 1 (Trivial) | ~3 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/forms-and-validation/src/index.tsx`:
- Line 125: Update the keyboard condition in the event handler to negate
event.ctrl with !event.ctrl rather than using a non-null assertion, so plain “c”
opens the modal while Ctrl+C follows the existing quit path.
In `@examples/pomodoro-timer/src/index.tsx`:
- Line 185: Update the percentage calculation in the label expression of the
timer class so the epsilon is applied at the normalized-value scale before
multiplying by 100, or otherwise use an equivalent scale-appropriate epsilon.
Preserve the existing conditional label behavior and rounded percentage output.
🪄 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: 849826da-4bd4-4f55-a874-1a51b68cf30e
📒 Files selected for processing (3)
examples/forms-and-validation/src/index.tsxexamples/pomodoro-timer/src/index.tsxexamples/widget-gallery/src/index.ts
| } | ||
|
|
||
| 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
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'examples/forms-and-validation/src/index\.tsx$' || true
echo
echo "Relevant lines:"
sed -n '105,135p' examples/forms-and-validation/src/index.tsx | cat -n
echo
echo "Package/version hints:"
if [ -f package.json ]; then jq -r '.devDependencies.typescript // .dependencies.typescript // empty' package.json; fi
if [ -f examples/forms-and-validation/package.json ]; then jq -r '.devDependencies.typescript // .dependencies.typescript // empty' examples/forms-and-validation/package.json; fi
echo
echo "Search nearby key handler:"
rg -n "event\.key|quit|modal|ctrl" examples/forms-and-validation/src/index.tsxRepository: Karanjot786/TermUI
Length of output: 2818
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Package manager files:"
git ls-files | rg '(^|/)package-lock\.json$|(^|/)pnpm-lock\.yaml$|(^|/)yarn\.lock$|(^|/)bun\.lockb?$|(^|/)tsconfig\.json$' || true
echo
echo "TypeScript parser availability:"
node - <<'JS'
const path = require.resolve('typescript');
const ts = require(path);
console.log('typescript path:', path);
console.log('typescript version:', ts.version);
const source = 'x.key === "c" && event.ctrl !;';
console.log(source);
const sf = ts.createSourceFile('a.ts', source, ts.ScriptTarget.Latest, true);
sf.forEachChild((n) => console.log(n.kind, ts.SyntaxKind[n.kind], n.getText().trim()));
console.log('parsed successfully');
JSRepository: Karanjot786/TermUI
Length of output: 2505
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Bun lock TypeScript entry:"
rg -n "typescript|tsx" bun.lock | head -40
echo
echo "Root/package example tsconfigs:"
for f in package.json examples/forms-and-validation/tsconfig.json; do
echo "--- $f"
sed -n '1,220p' "$f"
doneRepository: Karanjot786/TermUI
Length of output: 3263
Negate event.ctrl instead of asserting it non-null.
event.ctrl ! is a non-null assertion and does not test for the Control key. Use !event.ctrl so plain c can open the modal and Ctrl+C can keep using the quit path.
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.
Suggested change
| 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 to negate event.ctrl with !event.ctrl rather than
using a non-null assertion, so plain “c” opens the modal while Ctrl+C follows
the existing quit path.
Source: Coding guidelines
| const attrs = styleToCellAttrs(this._style); | ||
|
|
||
| const label = this._showLabel ? ` ${Math.round(this._value * 100)}%` : ''; | ||
| const label = this._showLabel ? ` ${Math.round(this._value * 100 + Number.EPSILON)}%` : ''; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
if [ -f examples/pomodoro-timer/src/index.tsx ]; then
nl -ba examples/pomodoro-timer/src/index.tsx | sed -n '140,205p'
else
echo "missing examples/pomodoro-timer/src/index.tsx"
fi
echo
echo "== JS Node probe for exact value and proposed forms =="
node - <<'JS'
const value = 0.145;
const beforeRound = value * 100 + Number.EPSILON;
const proposedRound = (value + Number.EPSILON) * 100;
console.log(JSON.stringify({
value: value,
valueTimes100: value * 100,
eps: Number.EPSILON,
beforeRoundString: beforeRound.toString(),
beforeRoundValue: beforeRound,
beforeRound: Math.round(beforeRound),
proposedRoundString: proposedRound.toString(),
proposedRound: Math.round(proposedRound),
exactThresholdBelowBy: Number.isFinite((100 * value) - 14.5) ? (100 * value) - 14.5 : 'not finite',
beforeRoundBelowThresholdBy: beforeRound - 14.5,
proposedRoundAboveThresholdBy: proposedRound - 14.5,
}, null, 2));
JSRepository: Karanjot786/TermUI
Length of output: 215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file exists and line count =="
if [ -f examples/pomodoro-timer/src/index.tsx ]; then
wc -l examples/pomodoro-timer/src/index.tsx
else
echo "missing examples/pomodoro-timer/src/index.tsx"
exit 0
fi
echo
echo "== file excerpt using sed =="
sed -n '140,205p' examples/pomodoro-timer/src/index.tsx
echo
echo "== JS Node probe for exact value and proposed forms =="
node - <<'JS'
const value = 0.145;
const beforeRound = value * 100 + Number.EPSILON;
const proposedRound = (value + Number.EPSILON) * 100;
console.log(JSON.stringify({
value: value,
valueTimes100: value * 100,
eps: Number.EPSILON,
beforeRoundString: beforeRound.toString(),
beforeRoundValue: beforeRound,
beforeRound: Math.round(beforeRound),
proposedRoundString: proposedRound.toString(),
proposedRound: Math.round(proposedRound),
exactThresholdBelowBy: Number.isFinite((100 * value) - 14.5) ? (100 * value) - 14.5 : 'not finite',
beforeRoundBelowThresholdBy: beforeRound - 14.5,
proposedRoundAboveThresholdBy: proposedRound - 14.5,
}, null, 2));
JSRepository: Karanjot786/TermUI
Length of output: 3166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== search render/setValue usages in pomodoro example =="
rg -n "setValue|get value|label|0\.145|14\.5|GradientProgressBar|_value" examples/pomodoro-timer/src -S || true
echo
echo "== probe percentage rounding edge cases for multiply-then-add vs add-then-multiply =="
node - <<'JS'
function roundScale(value) { return Math.round(value * 100 + Number.EPSILON); }
function roundProposed(value) { return Math.round((value + Number.EPSILON) * 100); }
const xs = [0.145, 0.285, 0.425, 0.565, 0.705, 0.845, 0.985];
for (const x of xs) {
console.log(`${x}: multiplyThenAdd=${roundScale(x)} addThenMultiply=${roundProposed(x)} diff=${roundScale(x)-roundProposed(x)}`);
}
JSRepository: Karanjot786/TermUI
Length of output: 2499
Scale Number.EPSILON before rounding the percentage.
Line 185 adds an absolute epsilon after this._value * 100, which is too small at that scale. A value like 0.145 rounds to 14%; add the epsilon to the normalized value before multiplying, or round the percent value with a scale-appropriate epsilon.
Proposed fix
- const label = this._showLabel ? ` ${Math.round(this._value * 100 + Number.EPSILON)}%` : '';
+ const label = this._showLabel ? ` ${Math.round((this._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.
Suggested change
| const label = this._showLabel ? ` ${Math.round(this._value * 100 + Number.EPSILON)}%` : ''; | |
| const label = this._showLabel ? ` ${Math.round((this._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/pomodoro-timer/src/index.tsx` at line 185, Update the percentage
calculation in the label expression of the timer class so the epsilon is applied
at the normalized-value scale before multiplying by 100, or otherwise use an
equivalent scale-appropriate epsilon. Preserve the existing conditional label
behavior and rounded percentage output.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes real bugs found in the codebase:
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.parseInt: without10, strings like'0x1F'or'08'parse in unintended bases.parseInt: without10, strings like'0x1F'or'08'parse in unintended bases.Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3594
Summary by CodeRabbit