fix: resolve 4 bugs in termui - #3683
Conversation
github-actions
Bot
added
type:bug
📝 WalkthroughWalkthroughThe changes update a form shortcut condition, make tab-key parsing explicitly decimal, and improve FPS rounding with ChangesCorrectness fixes
Estimated code review effort: 2 (Simple) | ~5 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: 1
🧹 Nitpick comments (1)
packages/dev-server/src/devtools.ts (1)
81-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the FPS rounding boundary.
The existing
recordRendertest inpackages/dev-server/src/devtools.test.ts, Lines [308]-[313], checks hover-state cleanup only. Add a test that controls render timestamps and asserts the reported FPS at a one-decimal rounding boundary. This verifies that theNumber.EPSILONchange prevents the targeted regression.🤖 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/dev-server/src/devtools.ts` at line 81, Add a regression test alongside the existing recordRender test in devtools.test.ts that controls render timestamps to produce an FPS value at a one-decimal rounding boundary, then assert the reported FPS uses the expected rounded value. Keep the test focused on the Number.EPSILON behavior in recordRender rather than hover-state cleanup.
🤖 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 clear-shortcut condition in the event-key handler to use
logical negation of event.ctrl, ensuring unmodified “c” opens the modal while
Ctrl+C remains unaffected.
---
Nitpick comments:
In `@packages/dev-server/src/devtools.ts`:
- Line 81: Add a regression test alongside the existing recordRender test in
devtools.test.ts that controls render timestamps to produce an FPS value at a
one-decimal rounding boundary, then assert the reported FPS uses the expected
rounded value. Keep the test focused on the Number.EPSILON behavior in
recordRender rather than hover-state cleanup.
🪄 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: f332fd9e-0e17-44ac-988a-58d83a30ef49
📒 Files selected for processing (3)
examples/forms-and-validation/src/index.tsxexamples/showcase/src/index.tsxpackages/dev-server/src/devtools.ts
| } | ||
|
|
||
| if (event.key === 'c' && event.ctrl === false) { | ||
| if (event.key === 'c' && event.ctrl !) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the key section without running repository code.
printf 'File candidates:\n'
fd -a 'index\.tsx$' . | sed 's#^\./##' | grep 'examples/forms-and-validation/src/index.tsx' || true
printf '\nFile size:\n'
wc -l examples/forms-and-validation/src/index.tsx
printf '\nRelevant lines 110-130:\n'
sed -n '110,130p' examples/forms-and-validation/src/index.tsx | nl -ba -v110
printf '\nSearch shortcut-related lines:\n'
rg -n "event\.key|quit|clear|Ctrl|ctrl|modal|key==='c'|key === 'c'" examples/forms-and-validation/src/index.tsxRepository: Karanjot786/TermUI
Length of output: 362
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check TypeScript availability for a deterministic syntax/scope probe only if needed.
if command -v tsc >/dev/null 2>&1; then
tsc --version
else
echo 'tsc not installed'
fiRepository: Karanjot786/TermUI
Length of output: 170
Use logical negation for the clear shortcut.
event.ctrl ! is a postfix non-null assertion, not !event.ctrl. Unmodified c therefore does not open the modal. Ctrl+C exits before this condition runs.
- 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, Update the
clear-shortcut condition in the event-key handler to use logical negation of
event.ctrl, ensuring unmodified “c” opens the modal while Ctrl+C remains
unaffected.
1 participant
Description
This PR fixes real bugs found in the codebase:
trim()to''misses whitespace-only input;.trim().length === 0is explicit.Number.EPSILONtoMath.round: prevents floating-point drift (e.g.1.005 * 100rounding to 100 instead of 101).parseInt: without10, strings like'0x1F'or'08'parse in unintended bases.x === trueis equivalent tox(andx === falseto!x), and shorter to read.Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #3682
Summary by CodeRabbit